[CRIU] [PATCH 12/12] page-xfer: Sanitize xfer core routine

[Pavel Emelyanov]

On 06/28/2017 07:28 PM, Mike Rapoport wrote:
> On Wed, Jun 28, 2017 at 04:48:45PM +0300, Pavel Emelyanov wrote:
>> Make it call .write_pagemap once and decide whether or not to
>> call .write_pages based on the flags caluculated in one place
>> as well.
>>
>> Signed-off-by: Pavel Emelyanov <xemul at virtuozzo.com>
>> ---
>>  criu/page-xfer.c | 23 ++++++++++++-----------
>>  1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/criu/page-xfer.c b/criu/page-xfer.c
>> index 10da75f..256cac6 100644
>> --- a/criu/page-xfer.c
>> +++ b/criu/page-xfer.c
>> @@ -405,6 +405,14 @@ static int dump_holes(struct page_xfer *xfer, struct page_pipe *pp,
>>  	return 0;
>>  }
>>
>> +static inline u32 ppb_xfer_flags(struct page_xfer *xfer, struct page_pipe_buf *ppb)
>> +{
>> +	if (ppb->flags & PPB_LAZY)
>> +		return PE_LAZY | (xfer->transfer_lazy ? PE_PRESENT : 0);
> 
> Huh. Then maybe even
> 
> return ppb->flags & PPB_LAZY ? PE_LAZY | (xfer->transfer_lazy ? PE_PRESENT : 0) : PE_PRESENT;
> 
> ;-)

:D

> I understand that if's and braces are not Spartan enough, but can we at
> least switch the order:
> 
> 	return (xfer->transfer_lazy ? PE_PRESENT : 0) | PE_LAZY;
> 
> and add a comment, e.g.:
> 
> 	/*
> 	 * Pages that can be lazily restored are always marked as such. In
> 	 * the case we actually transfer them into image make them as
> 	 * present as well.
> 	 */

OK, will add.

>> +	else
>> +		return PE_PRESENT;
>> +}
>> +
>>  int page_xfer_dump_pages(struct page_xfer *xfer, struct page_pipe *pp)
>>  {
>>  	struct page_pipe_buf *ppb;
>> @@ -420,7 +428,7 @@ int page_xfer_dump_pages(struct page_xfer *xfer, struct page_pipe *pp)
>>
>>  		for (i = 0; i < ppb->nr_segs; i++) {
>>  			struct iovec iov = ppb->iov[i];
>> -			u32 flags = PE_PRESENT;
>> +			u32 flags;
>>
>>  			ret = dump_holes(xfer, pp, &cur_hole, iov.iov_base);
>>  			if (ret)
>> @@ -431,19 +439,12 @@ int page_xfer_dump_pages(struct page_xfer *xfer, struct page_pipe *pp)
>>  			pr_debug("\tp %p [%u]\n", iov.iov_base,
>>  					(unsigned int)(iov.iov_len / PAGE_SIZE));
>>
>> -			if (ppb->flags & PPB_LAZY) {
>> -				if (!xfer->transfer_lazy) {
>> -					if (xfer->write_pagemap(xfer, &iov, PE_LAZY))
>> -						return -1;
>> -					continue;
>> -				} else {
>> -					flags |= PE_LAZY;
>> -				}
>> -			}
>> +			flags = ppb_xfer_flags(xfer, ppb);
>>
>>  			if (xfer->write_pagemap(xfer, &iov, flags))
>>  				return -1;
>> -			if (xfer->write_pages(xfer, ppb->p[0], iov.iov_len))
>> +			if ((flags & PE_PRESENT) && xfer->write_pages(xfer,
>> +						ppb->p[0], iov.iov_len))
> 
> Huh again. A newline after && ?

Yup, I'll resend.

>>  				return -1;
>>  		}
>>  	}
>> -- 
>> 2.1.4
>>
>> _______________________________________________
>> CRIU mailing list
>> CRIU at openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu
>>
> 
> .
>