[libvirt] [PATCH v4 09/14] hostdev: Maintain a driver list of active mediated devices

[Erik Skultety]

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