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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 25 Mar 2014, Jan Beulich wrote:
> >>> On 25.03.14 at 16:10, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > On Tue, 25 Mar 2014, Jan Beulich wrote:
> >> >>> On 25.03.14 at 13:35, <stefano.stabellini@xxxxxxxxxxxxx> 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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel