[CRIU] [PATCH] lazy-pages: consolidate error handling for uffdio_copy and uffdio_zeropage

[Mike Rapoport]

On Fri, Sep 22, 2017 at 03:23:27PM -0700, Andrei Vagin wrote:
> On Fri, Sep 22, 2017 at 06:25:43PM +0300, Mike Rapoport wrote:
> > The errors set by both uffdio_copy and uffdio_zeropage are the same and
> > require the same handling. Let's use a helper function to handle the errors
> > returned by uffdio_copy and uffdio_zero.
> > 
> > Signed-off-by: Mike Rapoport <rppt at linux.vnet.ibm.com>
> > ---
> > 
> > The patch is agaist criu-dev but should apply to master just the same
> > 
> >  criu/uffd.c | 40 +++++++++++++++++++++++-----------------
> >  1 file changed, 23 insertions(+), 17 deletions(-)
> > 
> > diff --git a/criu/uffd.c b/criu/uffd.c
> > index 43dc0ce..22b65ce 100644
> > --- a/criu/uffd.c
> > +++ b/criu/uffd.c
> > @@ -705,6 +705,27 @@ static int handle_exit(struct lazy_pages_info *lpi)
> >  	return 0;
> >  }
> >  
> > +static int uffd_check_op_error(struct lazy_pages_info *lpi, const char *op,
> > +			       unsigned long len, unsigned long rc_len, int rc)
> > +{
> > +	if (rc) {
> > +		if (errno == ENOSPC || errno == ESRCH) {
> > +			handle_exit(lpi);
> > +			return 0;
> > +		}
> > +		if (rc_len != -EEXIST) {
> > +			lp_debug(lpi, "%s: rc:%d copy:%ld, errno:%d\n",
> > +				 op, rc, rc_len, errno);
> 
> Why do we use lp_debug instead of lp_perror?

I think for historical reasons. Do you want v2 or maybe you could change it
to lp_err while applying the patch? ;-)
 
> 
> > +			return -1;
> > +		}
> > +	} else if (rc_len != len) {
> > +		lp_err(lpi, "%s unexpected size %ld\n", op, rc_len);
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int uffd_copy(struct lazy_pages_info *lpi, __u64 address, int nr_pages)
> >  {
> >  	struct uffdio_copy uffdio_copy;
> > @@ -719,20 +740,8 @@ static int uffd_copy(struct lazy_pages_info *lpi, __u64 address, int nr_pages)
> >  
> >  	lp_debug(lpi, "uffd_copy: 0x%llx/%ld\n", uffdio_copy.dst, len);
> >  	rc = ioctl(lpi->lpfd.fd, UFFDIO_COPY, &uffdio_copy);
> > -	if (rc) {
> > -		if (errno == ENOSPC || errno == ESRCH) {
> > -			handle_exit(lpi);
> > -			return 0;
> > -		}
> > -		if (uffdio_copy.copy != -EEXIST) {
> > -			lp_debug(lpi, "uffd_copy: rc:%d copy:%Ld, errno:%d\n",
> > -				 rc, uffdio_copy.copy, errno);
> > -			return -1;
> > -		}
> > -	} else if (uffdio_copy.copy != len) {
> > -		lp_err(lpi, "UFFDIO_COPY unexpected size %Ld\n", uffdio_copy.copy);
> > +	if (uffd_check_op_error(lpi, "copy", len, uffdio_copy.copy, rc))
> >  		return -1;
> > -	}
> >  
> >  	lpi->copied_pages += nr_pages;
> >  
> > @@ -784,11 +793,8 @@ static int uffd_zero(struct lazy_pages_info *lpi, __u64 address, int nr_pages)
> >  
> >  	lp_debug(lpi, "zero page at 0x%llx\n", address);
> >  	rc = ioctl(lpi->lpfd.fd, UFFDIO_ZEROPAGE, &uffdio_zeropage);
> > -	if (rc) {
> > -		lp_perror(lpi, "zero page failed: %Ld",
> > -			  uffdio_zeropage.zeropage);
> > +	if (uffd_check_op_error(lpi, "zero", len, uffdio_zeropage.zeropage, rc))
> >  		return -1;
> > -	}
> >  
> >  	return 0;
> >  }
> > -- 
> > 2.7.4
> > 
> 

-- 
Sincerely yours,
Mike.