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

[Laine Stump]

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?



>
>>    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 at laine.org>
>> ---
>>   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.


>
>>   qemuDomainRemoveAuditDevice(virDomainObjPtr vm,
>>                               virDomainDeviceDefPtr detach,
>>                               bool success)