2:24 a.m.
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
2:24 a.m.
New subject: [libvirt] [PATCH 1/4] virsh.rst: Mention media update is not supported in attach-{disk, device}
Signed-off-by: Han Han <hhan(a)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
2:24 a.m.
New subject: [libvirt] [PATCH 2/4] libvirt: Remove comments of media update in virDomainAttachDeviceFlags
Signed-off-by: Han Han <hhan(a)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
2:24 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: Drop support of media update in live disk attach API
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(a)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
2:24 a.m.
New subject: [libvirt] [PATCH 4/4] libxl: Drop cdrom media update support in live disk attch APIs
Signed-off-by: Han Han <hhan(a)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
3:37 a.m.
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 :|
1773
days inactive
1773
days old
5 comments
2 participants
participants (2)
-
Daniel P. Berrangé -
Han Han