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

[Andrew Vagin]

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.


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