pstree: rename lookup_create_{item <=> pid}

Submitted by Dmitry Safonov on Nov. 14, 2016, 6:36 p.m.

Details

Message ID 20161114183644.15326-1-dsafonov@virtuozzo.com
State Superseded
Series "pstree: rename lookup_create_{item <=> pid}"
Headers show

Commit Message

Dmitry Safonov Nov. 14, 2016, 6:36 p.m.
I suggest to rename lookup_create_pid into lookup_create_item and
backwards. I think, the naming is misleading, now *_pid() looks for/creates
pstree item, which can be thread, undef task or anything else,
while *_item() can't create/locate threads (because of BUG_ON()).
Let's swap them:
- lookup_create_item() will return pstree item - anything, process or thread
- lookup_create_pid() will only lookup/create processes, bugging on threads.

Cc: Andrey Vagin <avagin@virtuozzo.com>
Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
---
 criu/files-reg.c      |  2 +-
 criu/include/pstree.h |  2 +-
 criu/pstree.c         | 22 +++++++++++-----------
 3 files changed, 13 insertions(+), 13 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/files-reg.c b/criu/files-reg.c
index 1c3d32e0cbf7..047be3723857 100644
--- a/criu/files-reg.c
+++ b/criu/files-reg.c
@@ -361,7 +361,7 @@  static int open_remap_dead_process(struct reg_file_info *rfi,
 {
 	struct pstree_item *helper;
 
-	helper = lookup_create_item(rfe->remap_id);
+	helper = lookup_create_pid(rfe->remap_id);
 	if (!helper)
 		return -1;
 
diff --git a/criu/include/pstree.h b/criu/include/pstree.h
index 5768b645021c..349ef4830061 100644
--- a/criu/include/pstree.h
+++ b/criu/include/pstree.h
@@ -74,7 +74,7 @@  extern struct pstree_item *__alloc_pstree_item(bool rst);
 #define alloc_pstree_item() __alloc_pstree_item(false)
 extern void init_pstree_helper(struct pstree_item *ret);
 
-extern struct pstree_item *lookup_create_item(pid_t pid);
+extern struct pstree_item *lookup_create_pid(pid_t pid);
 extern void pstree_insert_pid(pid_t pid, struct pid *pid_node);
 extern struct pid *pstree_pid_by_virt(pid_t pid);
 
diff --git a/criu/pstree.c b/criu/pstree.c
index 943518d98fea..081d4d000e1c 100644
--- a/criu/pstree.c
+++ b/criu/pstree.c
@@ -366,9 +366,9 @@  static int prepare_pstree_for_shell_job(void)
 			pi->sid = current_sid;
 	}
 
-	if (lookup_create_item(current_sid) == NULL)
+	if (lookup_create_pid(current_sid) == NULL)
 		return -1;
-	if (lookup_create_item(current_gid) == NULL)
+	if (lookup_create_pid(current_gid) == NULL)
 		return -1;
 
 	return 0;
@@ -379,7 +379,7 @@  static int prepare_pstree_for_shell_job(void)
  * it is not there yet. If pid_node isn't set, pstree_item
  * is inserted.
  */
-static struct pid *lookup_create_pid(pid_t pid, struct pid *pid_node)
+static struct pid *lookup_create_item(pid_t pid, struct pid *pid_node)
 {
 	struct rb_node *node = pid_root_rb.rb_node;
 	struct rb_node **new = &pid_root_rb.rb_node;
@@ -415,16 +415,16 @@  void pstree_insert_pid(pid_t pid, struct pid *pid_node)
 {
 	struct pid* n;
 
-	n = lookup_create_pid(pid, pid_node);
+	n = lookup_create_item(pid, pid_node);
 
 	BUG_ON(n != pid_node);
 }
 
-struct pstree_item *lookup_create_item(pid_t pid)
+struct pstree_item *lookup_create_pid(pid_t pid)
 {
 	struct pid *node;;
 
-	node = lookup_create_pid(pid, NULL);
+	node = lookup_create_item(pid, NULL);
 	if (!node)
 		return NULL;
 	BUG_ON(node->state == TASK_THREAD);
@@ -492,7 +492,7 @@  static int read_pstree_image(pid_t *pid_max)
 			break;
 
 		ret = -1;
-		pi = lookup_create_item(e->pid);
+		pi = lookup_create_pid(e->pid);
 		if (pi == NULL)
 			break;
 		BUG_ON(pi->pid.state != TASK_UNDEF);
@@ -503,9 +503,9 @@  static int read_pstree_image(pid_t *pid_max)
 		 * be initialized when we meet PstreeEntry with this pid or
 		 * we will create helpers for them.
 		 */
-		if (lookup_create_item(e->pgid) == NULL)
+		if (lookup_create_pid(e->pgid) == NULL)
 			break;
-		if (lookup_create_item(e->sid) == NULL)
+		if (lookup_create_pid(e->sid) == NULL)
 			break;
 
 		pi->pid.virt = e->pid;
@@ -556,7 +556,7 @@  static int read_pstree_image(pid_t *pid_max)
 			pi->threads[i].state = TASK_THREAD;
 			if (i == 0)
 				continue; /* A thread leader is in a tree already */
-			node = lookup_create_pid(pi->threads[i].virt, &pi->threads[i]);
+			node = lookup_create_item(pi->threads[i].virt, &pi->threads[i]);
 
 			BUG_ON(node == NULL);
 			if (node != &pi->threads[i]) {
@@ -639,7 +639,7 @@  static int prepare_pstree_ids(void)
 			pid = get_free_pid();
 			if (pid < 0)
 				break;
-			helper = lookup_create_item(pid);
+			helper = lookup_create_pid(pid);
 			if (helper == NULL)
 				return -1;
 

Comments

Pavel Emelianov Nov. 17, 2016, 8:27 a.m.
On 11/14/2016 09:36 PM, Dmitry Safonov wrote:
> I suggest to rename lookup_create_pid into lookup_create_item and
> backwards. I think, the naming is misleading, now *_pid() looks for/creates
> pstree item, which can be thread, undef task or anything else,

But at the same time lookup_create_pid returns struct pid *, lookup_create_item
returns struct pstree_item and you suggest to mess it up :)

> while *_item() can't create/locate threads (because of BUG_ON()).
> Let's swap them:
> - lookup_create_item() will return pstree item - anything, process or thread
> - lookup_create_pid() will only lookup/create processes, bugging on threads.
> 
> Cc: Andrey Vagin <avagin@virtuozzo.com>
> Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
> ---
>  criu/files-reg.c      |  2 +-
>  criu/include/pstree.h |  2 +-
>  criu/pstree.c         | 22 +++++++++++-----------
>  3 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/criu/files-reg.c b/criu/files-reg.c
> index 1c3d32e0cbf7..047be3723857 100644
> --- a/criu/files-reg.c
> +++ b/criu/files-reg.c
> @@ -361,7 +361,7 @@ static int open_remap_dead_process(struct reg_file_info *rfi,
>  {
>  	struct pstree_item *helper;
>  
> -	helper = lookup_create_item(rfe->remap_id);
> +	helper = lookup_create_pid(rfe->remap_id);
>  	if (!helper)
>  		return -1;
>  
> diff --git a/criu/include/pstree.h b/criu/include/pstree.h
> index 5768b645021c..349ef4830061 100644
> --- a/criu/include/pstree.h
> +++ b/criu/include/pstree.h
> @@ -74,7 +74,7 @@ extern struct pstree_item *__alloc_pstree_item(bool rst);
>  #define alloc_pstree_item() __alloc_pstree_item(false)
>  extern void init_pstree_helper(struct pstree_item *ret);
>  
> -extern struct pstree_item *lookup_create_item(pid_t pid);
> +extern struct pstree_item *lookup_create_pid(pid_t pid);
>  extern void pstree_insert_pid(pid_t pid, struct pid *pid_node);
>  extern struct pid *pstree_pid_by_virt(pid_t pid);
>  
> diff --git a/criu/pstree.c b/criu/pstree.c
> index 943518d98fea..081d4d000e1c 100644
> --- a/criu/pstree.c
> +++ b/criu/pstree.c
> @@ -366,9 +366,9 @@ static int prepare_pstree_for_shell_job(void)
>  			pi->sid = current_sid;
>  	}
>  
> -	if (lookup_create_item(current_sid) == NULL)
> +	if (lookup_create_pid(current_sid) == NULL)
>  		return -1;
> -	if (lookup_create_item(current_gid) == NULL)
> +	if (lookup_create_pid(current_gid) == NULL)
>  		return -1;
>  
>  	return 0;
> @@ -379,7 +379,7 @@ static int prepare_pstree_for_shell_job(void)
>   * it is not there yet. If pid_node isn't set, pstree_item
>   * is inserted.
>   */
> -static struct pid *lookup_create_pid(pid_t pid, struct pid *pid_node)
> +static struct pid *lookup_create_item(pid_t pid, struct pid *pid_node)
>  {
>  	struct rb_node *node = pid_root_rb.rb_node;
>  	struct rb_node **new = &pid_root_rb.rb_node;
> @@ -415,16 +415,16 @@ void pstree_insert_pid(pid_t pid, struct pid *pid_node)
>  {
>  	struct pid* n;
>  
> -	n = lookup_create_pid(pid, pid_node);
> +	n = lookup_create_item(pid, pid_node);
>  
>  	BUG_ON(n != pid_node);
>  }
>  
> -struct pstree_item *lookup_create_item(pid_t pid)
> +struct pstree_item *lookup_create_pid(pid_t pid)
>  {
>  	struct pid *node;;
>  
> -	node = lookup_create_pid(pid, NULL);
> +	node = lookup_create_item(pid, NULL);
>  	if (!node)
>  		return NULL;
>  	BUG_ON(node->state == TASK_THREAD);
> @@ -492,7 +492,7 @@ static int read_pstree_image(pid_t *pid_max)
>  			break;
>  
>  		ret = -1;
> -		pi = lookup_create_item(e->pid);
> +		pi = lookup_create_pid(e->pid);
>  		if (pi == NULL)
>  			break;
>  		BUG_ON(pi->pid.state != TASK_UNDEF);
> @@ -503,9 +503,9 @@ static int read_pstree_image(pid_t *pid_max)
>  		 * be initialized when we meet PstreeEntry with this pid or
>  		 * we will create helpers for them.
>  		 */
> -		if (lookup_create_item(e->pgid) == NULL)
> +		if (lookup_create_pid(e->pgid) == NULL)
>  			break;
> -		if (lookup_create_item(e->sid) == NULL)
> +		if (lookup_create_pid(e->sid) == NULL)
>  			break;
>  
>  		pi->pid.virt = e->pid;
> @@ -556,7 +556,7 @@ static int read_pstree_image(pid_t *pid_max)
>  			pi->threads[i].state = TASK_THREAD;
>  			if (i == 0)
>  				continue; /* A thread leader is in a tree already */
> -			node = lookup_create_pid(pi->threads[i].virt, &pi->threads[i]);
> +			node = lookup_create_item(pi->threads[i].virt, &pi->threads[i]);
>  
>  			BUG_ON(node == NULL);
>  			if (node != &pi->threads[i]) {
> @@ -639,7 +639,7 @@ static int prepare_pstree_ids(void)
>  			pid = get_free_pid();
>  			if (pid < 0)
>  				break;
> -			helper = lookup_create_item(pid);
> +			helper = lookup_create_pid(pid);
>  			if (helper == NULL)
>  				return -1;
>  
>
Dmitry Safonov Nov. 17, 2016, 9:35 a.m.
On 11/17/2016 11:27 AM, Pavel Emelyanov wrote:
> On 11/14/2016 09:36 PM, Dmitry Safonov wrote:
>> I suggest to rename lookup_create_pid into lookup_create_item and
>> backwards. I think, the naming is misleading, now *_pid() looks for/creates
>> pstree item, which can be thread, undef task or anything else,
>
> But at the same time lookup_create_pid returns struct pid *, lookup_create_item
> returns struct pstree_item and you suggest to mess it up :)

Oh, yes - didn't catch that.
Maybe something like lookup_create_process()?
Or maybe leave as is - I just find it a little confusing that sounding
"more generic" _create_item() doesn't create threads %)
...while _create_pid() does.
Pavel Emelianov Nov. 17, 2016, 10:42 a.m.
On 11/17/2016 12:35 PM, Dmitry Safonov wrote:
> On 11/17/2016 11:27 AM, Pavel Emelyanov wrote:
>> On 11/14/2016 09:36 PM, Dmitry Safonov wrote:
>>> I suggest to rename lookup_create_pid into lookup_create_item and
>>> backwards. I think, the naming is misleading, now *_pid() looks for/creates
>>> pstree item, which can be thread, undef task or anything else,
>>
>> But at the same time lookup_create_pid returns struct pid *, lookup_create_item
>> returns struct pstree_item and you suggest to mess it up :)
> 
> Oh, yes - didn't catch that.
> Maybe something like lookup_create_process()?
> Or maybe leave as is - I just find it a little confusing that sounding
> "more generic" _create_item() doesn't create threads %)
> ...while _create_pid() does.

Yup, some better names are welcome.
Maybe lookup_create_pstree_item?
This is exactly what it, you know, looks up and, well, creates.

-- Pavel