[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 4/7] xen, common: add the XEN_DOMCTL_memory_mapping hypercall
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@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. Although the reuse of the add variable in the else clause if an if (add) is pretty damned confusing... Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |