[CRIU] [PATCH] kcmp: Fix ret code comparison, v2

[Andrew Vagin]

On Tue, Apr 22, 2014 at 10:37:22AM +0400, Cyrill Gorcunov wrote:
> On Tue, Apr 22, 2014 at 10:31:13AM +0400, Cyrill Gorcunov wrote:
> > On Tue, Apr 22, 2014 at 08:43:03AM +0400, Andrew Vagin wrote:
> > > > @@ -149,12 +149,14 @@ static u32 kid_generate_sub(struct kid_tree *tree, struct kid_entry *e,
> > > >  				this->elem.idx, elem->idx);
> > > >  
> > > >  		parent = *new;
> > > > -		if (ret < 0)
> > > Can we print an error here?
> > > 			pr_perror("Unable to compare (%d, %x) %x (%d, %x)", ....)
> > 
> > Sure, I guess instead of BUG better to exit with error. Will update, thanks.
> 
> Attached.

Acked-by: Andrew Vagin <avagin at parallels.com>

> From a023d6c787828366dc68661bab595aedbe3a1b07 Mon Sep 17 00:00:00 2001
> From: Cyrill Gorcunov <gorcunov at openvz.org>
> Date: Tue, 22 Apr 2014 00:33:29 +0400
> Subject: [PATCH] kcmp: Fix ret code comparison
> 
> In the early draft of kcmp syscall it has been returning
> [-1|0|1] values but finally [0|1|2] were merged into the
> kernel, but I forgot to update the criu code. The good
> thing is that because we're using rbtree the kcmp results
> are still sorted and tree is balanced but sometime we may
> take a wrong branch generating new ID even if the object
> is present in the tree which eventually may lead to dump
> faulure.
> 
> Reported-by: Deyan Doychev <deyan at 1h.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>
> ---
>  kcmp-ids.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/kcmp-ids.c b/kcmp-ids.c
> index 3f75cff33f46..f082d9730f59 100644
> --- a/kcmp-ids.c
> +++ b/kcmp-ids.c
> @@ -149,12 +149,18 @@ static u32 kid_generate_sub(struct kid_tree *tree, struct kid_entry *e,
>  				this->elem.idx, elem->idx);
>  
>  		parent = *new;
> -		if (ret < 0)
> +		if (ret == 1)
>  			node = node->rb_left, new = &((*new)->rb_left);
> -		else if (ret > 0)
> +		else if (ret == 2)
>  			node = node->rb_right, new = &((*new)->rb_right);
> -		else
> +		else if (ret == 0)
>  			return this->subid;
> +		else {
> +			pr_err("kcmp failed: pid (%d %d) type %u idx (%u %u) ret %d\n",
> +			       this->elem.pid, elem->pid, tree->kcmp_type,
> +			       this->elem.idx, elem->idx, ret);
> +			return 0;
> +		}
>  	}
>  
>  	sub = alloc_kid_entry(tree, elem);
> -- 
> 1.8.3.1
>