Re: [PATCH 20/21] qemu_hotplug: consolidate all common detach code in qemuDomainDetachDeviceLive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/22/19 10:14 AM, Peter Krempa wrote:
On Fri, Mar 22, 2019 at 10:08:17 -0400, Laine Stump wrote:
On 3/22/19 8:32 AM, Peter Krempa wrote:
On Thu, Mar 21, 2019 at 18:29:00 -0400, Laine Stump wrote:
Now that all the qemuDomainDetachPrep*() functions look nearly
identical at the end, we can put one copy of that identical code in
qemuDomainDetachDeviceLive() at the point after the individual prep
functions have been called, and remove the duplicated code from all
the prep functions. The code to locate the target "detach" device
based on the "match" device remains, as do all device-type-specific
validations.

Unfortunately there are a few things going on at once in this patch,
which makes it a bit more difficult to follow than the others; it was
just impossible to do the changes in stages and still have a
buildable/testable tree at each step.

The other changes of note:

* The individual prep functions no longer need their driver or async
    args, so those are removed, as are the local "ret" variables, since
    in all cases the functions just directly return -1 or 0.

* Some of the prep functions were checking for a valid alias and/or
    for attempts to detach a multifunction PCI device, but not all. In
    fact, both checks are valid (or at least harmless) for *all* device
    types, so they are removed from the prep functions, and done a
    single time in the common function.

    (any attempts to *create* an alias when there isn't one has been
    removed, since that is doomed to failure anyway; the only way the
    device wouldn't have an alias is if 1) the domain was created by
    calling virsh qemu-attach to attach an existing qemu process to
    libvirt, and 2) the qemu command that started said process used "old
    style" arguments for creating devices that didn't have any device
    ids. Even if we constructed a device id for one of these devices,
    qemu wouldn't recognize it in the device_del command anyway, so we
    may as well fail earlier with "device missing alias" rather than
    failing later with "couldn't delete device net0".)

* Only one type of device has shutdown code that must not be called
    until after *all* validation of the device is done (including
Why don't we put that to the RemoveDevice handler for network? We
teardown all related stuff usually after unplug of the frontend.

That is a good question! I'm not sure why network devices have this part of
their shutdown handled early. I don't think we should make that change here
though. I can insert an earlier patch to do it, hoping that it won't break
anything, or I can make a later patch that moves it after this change, which
seems kind of superfluous, but would make it simpler to revert in case it
causes a regression. What's your opinion?
I honestly have no idea what that cleanup is done for network devices,
it just feels wrong to teardown stuff prior to the OS even giving up the
device.


iirc, bandwidth rules are removed, and the interface is taken offline. Most like it's just that way because the original code had those happening early by coincidence, and then later when it was split into the separate detach and remove pieces, it got left in detach.



Changing it prior will result in cleaner code, fixing it after will be
easily revertable. I'd probably go for the latter.


Yeah, especially since I already made the patch that splits it out, etc. :-)





    checking for multifunction PCI and valid alias, which is done in the
    toplevel common code). For this reason, the Net function has been
    split in two, with the 2nd half (qemuDomainDetachShutdownNet())
    called from the common function, right before sending the delete
    command to qemu.

Signed-off-by: Laine Stump <laine@xxxxxxxxx>
---
   src/qemu/qemu_hotplug.c | 497 ++++++++++++----------------------------
   1 file changed, 149 insertions(+), 348 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 5e5ffe16d3..de7a7a2c95 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5208,7 +5208,7 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
   }
-static inline void
+static void
Use ATTRIBUTE_UNUSED rather than inline.

I couldn't remember where was the proper place to put ATTRIBUTE_UNUSED in
the function definition, and no matter where I put it it didn't work, so I
gave up and used inline. Obviously that doesn't work for all compilers
though, so I'll figure out the right place.
static void ATTRIBUTE_UNUSED
virAsdf(int qwer)
{
}


Hmmph. I could *swear* that was the first one I tried, and I got an error. Maybe I wasn't paying close attention, and it was an error about some other detail that I coincidentally fixed at the same time I switched to using inline.


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux