[libvirt] [PATCH 0/4][RFC] Drop media update support in virDomainAttachDevice*
https://bugzilla.redhat.com/show_bug.cgi?id=1788793 Before virDomainUpdateDeviceFlags, virDomainAttachDevice* were used for media update. Now we have introduced virDomainUpdateDeviceFlags for 9 years(since commit 46a2ea3689) but keep that compatibility of virDomainAttachDevice* until now. I think it is time to remove the compatibility since it doesn't work as well as virDomainUpdateDeviceFlags on disk xml validation. I wonder if any uplayer software relying on this compatibility... My git repo: https://github.com/qiankehan/libvirt/tree/attach-device-nocdrom Han Han (4): virsh.rst: Mention media update is not supported in attach-{disk,device} libvirt: Remove comments of media update in virDomainAttachDeviceFlags qemu: Drop support of media update in live disk attach API libxl: Drop cdrom media update support in live disk attch APIs docs/manpages/virsh.rst | 14 +++++++------- src/libvirt-domain.c | 4 ---- src/libxl/libxl_driver.c | 6 ++++-- src/qemu/qemu_hotplug.c | 12 ++++-------- 4 files changed, 15 insertions(+), 21 deletions(-) -- 2.24.0.rc1
Signed-off-by: Han Han <hhan@redhat.com> --- docs/manpages/virsh.rst | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index c637caa583..92f113fbde 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -4391,9 +4391,7 @@ as the top-level element. See the documentation at libvirt XML format for a device. If *--config* is specified the command alters the persistent domain configuration with the device attach taking effect the next time libvirt starts the domain. -For cdrom and floppy devices, this command only replaces the media -within an existing device; consider using ``update-device`` for this -usage. For passthrough host devices, see also ``nodedev-detach``, +For passthrough host devices, see also ``nodedev-detach``, needed if the PCI device does not use managed mode. If *--live* is specified, affect a running domain. @@ -4408,7 +4406,8 @@ an offline domain, and like *--live* *--config* for a running domain. ``Note``: using of partial device definition XML files may lead to unexpected results as some fields may be autogenerated and thus match devices other than -expected. +expected. It doesn't support replace the media within the existing virtual +cdrom or floppy device. Use ``update-device`` or ``change-media`` instead. attach-disk @@ -4442,9 +4441,7 @@ of the disk source, such as *raw* or *qcow2*. Hypervisor default will be used if *subdriver* is not specified. However, the default may not be correct, esp. for QEMU as for security reasons it is configured not to detect disk formats. *type* can indicate *lun*, *cdrom* or *floppy* as -alternative to the disk default, although this use only replaces the media -within the existing virtual cdrom or floppy device; consider using -``update-device`` for this usage instead. +alternative to the disk default. *alias* can set user supplied alias. *mode* can specify the two specific mode *readonly* or *shareable*. *sourcetype* can indicate the type of source (block|file) @@ -4476,6 +4473,9 @@ For compatibility purposes, *--persistent* behaves like *--config* for an offline domain, and like *--live* *--config* for a running domain. Likewise, *--shareable* is an alias for *--mode shareable*. +``Note``: It doesn't support replace the media within the existing virtual +cdrom or floppy device. Use ``update-device`` or ``change-media`` instead. + attach-interface ---------------- -- 2.24.0.rc1
Signed-off-by: Han Han <hhan@redhat.com> --- src/libvirt-domain.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index eb66999f07..a6a246c3a4 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -8208,10 +8208,6 @@ virDomainAttachDevice(virDomainPtr domain, const char *xml) * return failure if LIVE is specified but it only supports modifying the * persisted device allocation. * - * For compatibility, this method can also be used to change the media - * in an existing CDROM/Floppy device, however, applications are - * recommended to use the virDomainUpdateDeviceFlag method instead. - * * Be aware that hotplug changes might not persist across a domain going * into S4 state (also known as hibernation) unless you also modify the * persistent domain definition. -- 2.24.0.rc1
virDomainUpdateDeviceFlags has been introduced since v0.8.0. There is no reason to use virDomainAttachDevice* API to update media for cdrom or floopy device. https://bugzilla.redhat.com/show_bug.cgi?id=1788793 Signed-off-by: Han Han <hhan@redhat.com> --- src/qemu/qemu_hotplug.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 31d455505b..db8cb0e696 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1076,7 +1076,7 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriverPtr driver, * @vm: domain object * @dev: device to attach (expected type is DISK) * - * Attach a new disk or in case of cdroms/floppies change the media in the drive. + * Attach a new disk in the drive. * This function handles all the necessary steps to attach a new storage source * to the VM. */ @@ -1088,17 +1088,13 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, virDomainDiskDefPtr disk = dev->data.disk; virDomainDiskDefPtr orig_disk = NULL; - /* this API overloads media change semantics on disk hotplug - * for devices supporting media changes */ if ((disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM || disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) && (orig_disk = virDomainDiskByTarget(vm->def, disk->dst))) { - if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk, - disk->src, false) < 0) + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cdrom/floppy media update isn't supported. " + "Use virDomainUpdateDeviceFlags instead.")); return -1; - - disk->src = NULL; - return 0; } return qemuDomainAttachDeviceDiskLiveInternal(driver, vm, dev); -- 2.24.0.rc1
Signed-off-by: Han Han <hhan@redhat.com> --- src/libxl/libxl_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index f021ec9c5d..b481e950da 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3030,8 +3030,10 @@ libxlDomainAttachDeviceDiskLive(virDomainObjPtr vm, virDomainDeviceDefPtr dev) switch (l_disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: - ret = libxlDomainChangeEjectableMedia(vm, l_disk); - break; + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + "%s", _("cdrom update is not supported in this API. " + "Use virDomainUpdateDeviceFlags instead.")); + goto cleanup; case VIR_DOMAIN_DISK_DEVICE_DISK: if (l_disk->bus == VIR_DOMAIN_DISK_BUS_XEN) { if (virDomainDiskIndexByName(vm->def, l_disk->dst, true) >= 0) { -- 2.24.0.rc1
On Wed, Jan 15, 2020 at 04:24:54PM +0800, Han Han wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1788793
Before virDomainUpdateDeviceFlags, virDomainAttachDevice* were used for media update. Now we have introduced virDomainUpdateDeviceFlags for 9 years(since commit 46a2ea3689) but keep that compatibility of virDomainAttachDevice* until now.
I think it is time to remove the compatibility since it doesn't work as well as virDomainUpdateDeviceFlags on disk xml validation.
I wonder if any uplayer software relying on this compatibility...
NACK, this is explicitly the kind of thing we will never do. Providing a long term stable API means keeping existing methods operating unchanged, even if we have since introduced a new method which can do the same thing in a better way. The cost of keeping this existing code is negligible in context to the maint work of libvirt in general. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrangé -
Han Han