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

[Andrew Vagin]

On Fri, Sep 20, 2013 at 06:45:08PM +0400, Pavel Emelyanov wrote:
> 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);

ret contains an error code and I want to know it in case of any error.
I don't want argue about this minor thing. I'm agree that yours code is
more readable. I sent another version.

Thanks.

> 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
> > .
> > 
> 
>