On Sun, Mar 26, 2017 at 03:00:58PM -0400, Laine Stump wrote:
> (I'm unable to apply this patch to the head of master with "git am -3",
> and it won't show me the conflicts (it just fails saying "fatal: sha1
> information is lacking or useless (src/libvirt_private.syms), error:
> could not build fake ancestor". Because of this, all further review is
> based purely on examining the patch emails.)
Hmm, I also got a merge conflict (not sure if it's the same one you got) at
virHostdevReAttachMediatedDevices, but not with a fatal error. Wondering what
the problem at your end might be.
> BTW, I'm assuming that you've run "make syntax-check && make check" as
> each patch is applied, so that we're sure the functions in
I have.
> libvirt_private.syms are in proper alphabetic order (among other
> things). I've run it at a few stages of applying the patches, but not all.
>
>
[..]
> > void
> > +qemuHostdevReAttachMediatedDevices(virQEMUDriverPtr driver,
> > + const char *name,
> > + virDomainHostdevDefPtr *hostdevs,
> > + int nhostdevs)
> > +{
> > + virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
> > +
> > + virHostdevReAttachMediatedDevices(hostdev_mgr, QEMU_DRIVER_NAME,
> > + name, hostdevs, nhostdevs);
>
>
> What does "ReAttach" mean for a mediated device? Isn't this a NOP?
>
> Oh, nevermind - I looked ahead and compared to the PCI version of the
> ReAttach function. Turns out the PCI version does a *bunch* of things
> other than reattaching the device to its host driver, including
> restoring netdev config (which doesn't apply for mdevs and calling
> virPCIDeviceReset() (which is a NOP even for PCI devices as long as
> we're using VFIO, which *everybody* is these days). *BUT* the one other
> thing it does is move the devices that had been in use by the domain
> from the active list to the inactive list, and that's what the mdev
> version of the function does.
Yeah, this was merely just to stay consistent, I hated it from the very moment
I introduced the function, but I checked with other types of devices and
thought "it just might be easier to read", but I will add a comment that it's
there just because of consistency reasons and re-attach doesn't make any sense
for mdevs.
>
> Someday in the future we may want to clean these up and rename the
> functions appropriately, but for now having them named similarly makes
> it easier to review, so forget I said anything :-)
Yep.
Erik
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
