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