[libvirt] [PATCH 07/21] qemu_hotplug: refactor qemuDomainDetachDiskLive and qemuDomainDetachDiskDevice

[Laine Stump]

On 3/22/19 5:07 AM, Peter Krempa wrote:
> On Thu, Mar 21, 2019 at 18:28:47 -0400, Laine Stump wrote:
>> qemuDomainDetachDiskDevice() is only called from one place. Moving the
>> contents of the function to that place makes
>> qemuDomainDetachDiskLive() more similar to the other Detach functions
>> called by the toplevel qemuDomainDetachDevice().
>>
>> The goal is to make each of the device-type-specific functions do this:
>>
>>    1) find the exact device
>>    2) do any device-specific validation
>>    3) do general validation
>>    4) do device-specific shutdown (only needed for net devices)
>>    5) do the common block of code to send device_del to qemu, then
>>       optionally wait for a corresponding DEVICE_DELETED event from
>>       qemu.
>>
>> with the final aim being that only items 1 & 2 will remain in each
>> device-type-specific function, while 3 & 5 (which are the same for
>> almost every type) will be de-duplicated and moved to the toplevel
>> function that calls all of these (qemuDomainDetachDeviceLive(), which
>> will also contain a callout to the one instance of (4) (netdev).
> I'm not sure whether this is worth setting into ston^w commit message.


Yeah, that should go below the --, but I can never remember to do that 
when sending the patch, so I just include the comments in the commit 
message and remove it later. (Is there a way to add "meta comments"?)

>
>> Signed-off-by: Laine Stump <laine at laine.org>
>> ---
>>   src/qemu/qemu_hotplug.c | 96 ++++++++++++++++++-----------------------
>>   1 file changed, 43 insertions(+), 53 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index af99f3bf4c..820929b109 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
> [...]
>
>> @@ -5374,16 +5334,46 @@ qemuDomainDetachDeviceDiskLive(virQEMUDriverPtr driver,
>>       case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
>>           virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>>                          _("disk device type '%s' cannot be detached"),
>> -                       virDomainDiskDeviceTypeToString(disk->device));
>> -        break;
>> +                       virDomainDiskDeviceTypeToString(detach->device));
>> +        return -1;
>>   
>>       case VIR_DOMAIN_DISK_DEVICE_LAST:
>>       default:
>> -        virReportEnumRangeError(virDomainDiskDevice, disk->device);
>> +        virReportEnumRangeError(virDomainDiskDevice, detach->device);
>>           break;
> Missing conversion to 'return -1;'


Ooh!! Good eye!!


>
> ACK with the above fixed.