[CRIU] [PATCH] restorer: rework unmaping old VMA-s

[Pavel Emelyanov]

On 09/20/2013 06:38 PM, Andrew Vagin wrote:
> On Fri, Sep 20, 2013 at 06:33:12PM +0400, Pavel Emelyanov wrote:
>>> @@ -523,6 +523,37 @@ void __export_unmap(void)
>>>  }
>>>  
>>>  /*
>>> + * This function unmaps all VMAs, which don't belong to
>>> + * the restored process or the restorer
>>> + */
>>> +static int unmap_old_vmas(void *premmapped_addr, unsigned long premmapped_len,
>>> +		      void *bootstrap_start, unsigned long bootstrap_len)
>>> +{
>>> +	void *p[6] = {NULL, 0, 0, 0, 0, (void *) TASK_SIZE};
>>> +	int xchg, i;
>>> +
>>> +	/* Sorting vma-s */
>>> +	xchg = premmapped_addr > bootstrap_start ? 2 : 0;
>>> +
>>> +	p[1 + xchg] = premmapped_addr;
>>> +	p[2 + xchg] = premmapped_addr + premmapped_len;
>>> +	p[3 - xchg] = bootstrap_start;
>>> +	p[4 - xchg] = bootstrap_start + bootstrap_len;
>>> +
>>> +	for (i = 0; i < 6; i += 2) {
>>> +		int ret;
>>> +		ret = sys_munmap(p[i], p[i + 1] - p[i]);
>>> +		if (ret) {
>>> +			pr_err("Unable to unmap (%p-%p): %d\n",
>>> +						p[i], p[i + 1], ret);
>>> +			return -1;
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>
>> This is very hard to read. Why not make it simpler?
>>
>> if (premmapped_addr < bootstrap_addr) {
>> 	area_1_start = premmaped_addr;
>> 	area_1_end = premmaped_addr + premmapped_len;
>> 	area_2_start = bootstrap_start;
>> 	area_2_end = bootstrap_start + bootstrap_len;
>> } else {
>> 	area_1_start = bootstrap_start;
>> 	area_1_end = bootstrap_start + bootstrap_len;
>> 	area_2_start = premmaped_addr;
>> 	area_2_end = premmaped_addr + premmapped_len;
>> }
>>
>> sys_munmap(0,          area_1_start);
>> sys_munmap(area_1_end, area_2_start - area_1_end);
>> sys_munmap(area_2_end, TASK_SIZE - area_2_end);
> 
> You must handle errors for each munmap. I don't like these code
> duplication.

ret |= sys_munmap(0,          area_1_start);
ret |= sys_munmap(area_1_end, area_2_start - area_1_end);
ret |= sys_munmap(area_2_end, TASK_SIZE - area_2_end);
if (ret) {
	pr_perror();
	return -1;
}
 
> I had similar code ;):
> +               if ((void *) args->premmapped_addr < bootstrap_start) {
> +                       p1 = (void *) args->premmapped_addr;
> +                       s1 = args->premmapped_len;
> +                       p2 = bootstrap_start;
> +                       s2 = bootstrap_len;
> +               } else {
> +                       p2 = (void *) args->premmapped_addr;
> +                       s2 = args->premmapped_len;
> +                       p1 = bootstrap_start;
> +                       s1 = bootstrap_len;
>                 }
> 
> But I like more what I sent.
> 
>>
>>> +}
>>> +
>>> +/*
>>>   * The main routine to restore task via sigreturn.
>>>   * This one is very special, we never return there
>>>   * but use sigreturn facility to restore core registers
> .
>