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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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