Re: [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall

[Ian Campbell <Ian.Campbell@eu.citrix.com>]

On Tue, 2014-03-25 at 15:42 +0000, Stefano Stabellini wrote:
> On Tue, 25 Mar 2014, Jan Beulich wrote:
> > >>> On 25.03.14 at 16:10, <stefano.stabellini@eu.citrix.com> wrote:
> > > On Tue, 25 Mar 2014, Jan Beulich wrote:
> > >> >>> On 25.03.14 at 13:35, <stefano.stabellini@eu.citrix.com> wrote:
> > >> > On Tue, 25 Mar 2014, Arianna Avanzini wrote:
> > >> >> +            add = unmap_mmio_regions(d, gfn, gfn_end - 1, mfn);
> > >> >> +            ret = iomem_deny_access(d, mfn, mfn_end - 1);
> > >> >> +            if ( ret && add )
> > >> >> +                ret = -EIO;
> > >> > 
> > >> > This is a functional change. The removed code did:
> > >> > 
> > >> > -            ret = iomem_deny_access(d, mfn, mfn + nr_mfns - 1);
> > >> > -            if ( !ret && add )
> > >> > -                ret = -EIO;
> > >> > 
> > >> > The change looks correct to me, but I would appreciate if you could
> > >> > mention it explicitly in the commit message.
> > >> 
> > >> I overlooked this, and no, it is not correct (and I don't follow why
> > >> you think it is): We only want to force ret to -EIO if
> > >> iomem_deny_access() didn't already return another error.
> > > 
> > > I thought that the intention was to return -EIO if unmap_mmio_regions
> > > failed, even if iomem_deny_access is successful.
> > > iomem_deny_access errors should be taken care by the
> > > iomem_access_permitted check above, returning -EPERM.
> > 
> > It certainly doesn't matter _that_ much which error to return here,
> > but code movement shouldn't be done with silent functional changes,
> 
> I agree
> 
> 
> > the more that the change is buggy beyond altering original behavior:
> > Consider the case of ret = 0 (iomem_deny_access() succeeded) and
> > add = 1 (unmap_mmio_regions() failed) - the failure would be lost
> > with the changed code.
> 
> Yes, you are right, the original code is the correct one.

Although the reuse of the add variable in the else clause if an if (add)
is pretty damned confusing...

Ian.