Re: [libvirt] [PATCH 07/21] qemu_hotplug: refactor qemuDomainDetachDiskLive and qemuDomainDetachDiskDevice
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(a)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.