On 4/10/19 3:08 PM, Jim Fehlig wrote: > I noticed libvirt-tck test domain/207-disk-media-change.t started failing > after updating to libvirt 5.2.0. A bisection fingered commit f1d65853 > > commit f1d6585300001c7b23b8796a0faa4411c3531996 > Author: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; > Date:   Fri Mar 15 18:06:45 2019 -0300 > >     domain_conf: check device address before attach > > This commit prevents a media change on CDROM devices. Thanks for letting us know. I'll run libvirt-tck (any pointers are appreciated - first time I've heard about this test suite) and see if I can get a fix going.

On 4/10/19 3:33 PM, Jim Fehlig wrote: > On 4/10/19 12:25 PM, Daniel Henrique Barboza wrote: >> >> >> On 4/10/19 3:08 PM, Jim Fehlig wrote: >>> I noticed libvirt-tck test domain/207-disk-media-change.t started failing >>> after updating to libvirt 5.2.0. A bisection fingered commit f1d65853 >>> >>> commit f1d6585300001c7b23b8796a0faa4411c3531996 >>> Author: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; >>> Date:   Fri Mar 15 18:06:45 2019 -0300 >>> >>>     domain_conf: check device address before attach >>> >>> This commit prevents a media change on CDROM devices. >> >> Thanks for letting us know. I'll run libvirt-tck (any pointers are appreciated - >> first time I've heard about this test suite) and see if I can get a fix going. > > It's easy to reproduce outside of libvirt-tck. Simply start a VM with a cdrom > device and then try to update the <source> element with 'virsh attach-device > ...'. E.g. with the VM running try to "eject" the cdrom > > # cat update-empty-cdrom.xml > <disk type='file' device='cdrom'> >   <target dev='hdc'/> > </disk> > > # virsh attach-device test update-empty-cdrom.xml > error: Failed to attach device from update-empty-cdrom.xml > error: Requested operation is not valid: Domain already contains a device with > the same address To give a background on this patch, this was intended to avoid a situation where a hotplug of a device with the same ID would fall back to a cleanup code that ended up unplugging the previous existing device in the guest. This was happening when using the QEMU driver.

What I didn't expect is for a CD-ROM media to be changed using a attach-device command. I think that the CD-ROM type has specific rules of not hot-unplugging itself (i.e. the driver) in the situation I described above. What I was expecting for CDROM change media was a VIR_DOMAIN_DEVICE_ACTION_UPDATE action instead, so this usage completely went under the radar for that patch.

In fact, there are "update-device" and "change-media" commands that, according to the man page, are meant for the purpose of media change. However, the same man page doesn't prohibit the use of attach-device for media change either. I am not sure why have a change-media command if attach-device is supposed to handle media change as well, but for now let's fix the usage of attach-device with CDROM.

On 4/10/19 1:17 PM, Daniel Henrique Barboza wrote: > > > On 4/10/19 3:33 PM, Jim Fehlig wrote: > > On 4/10/19 12:25 PM, Daniel Henrique Barboza wrote: > > > > > > > > > On 4/10/19 3:08 PM, Jim Fehlig wrote: > > > > I noticed libvirt-tck test domain/207-disk-media-change.t > > > > started failing after updating to libvirt 5.2.0. A bisection > > > > fingered commit f1d65853 > > > > > > > > commit f1d6585300001c7b23b8796a0faa4411c3531996 > > > > Author: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; > > > > Date:   Fri Mar 15 18:06:45 2019 -0300 > > > > > > > >     domain_conf: check device address before attach > > > > > > > > This commit prevents a media change on CDROM devices. > > > > > > Thanks for letting us know. I'll run libvirt-tck (any pointers are appreciated - > > > first time I've heard about this test suite) and see if I can get a fix going. > > > > It's easy to reproduce outside of libvirt-tck. Simply start a VM > > with a cdrom device and then try to update the <source> element with > > 'virsh attach-device ...'. E.g. with the VM running try to "eject" > > the cdrom > > > > # cat update-empty-cdrom.xml > > <disk type='file' device='cdrom'> > >   <target dev='hdc'/> > > </disk> > > > > # virsh attach-device test update-empty-cdrom.xml > > error: Failed to attach device from update-empty-cdrom.xml > > error: Requested operation is not valid: Domain already contains a > > device with the same address > > > To give a background on this patch, this was intended to avoid a situation > where a hotplug of a device with the same ID would fall back to a cleanup > code that ended up unplugging the previous existing device in the guest. > This was happening when using the QEMU driver. Understood. And fixing that bug is a good thing! > What I didn't expect is for a CD-ROM media to be changed using a > attach-device command. I think that the CD-ROM type has specific rules of not > hot-unplugging itself (i.e. the driver) in the situation I described above. What > I was expecting for CDROM change media was a VIR_DOMAIN_DEVICE_ACTION_UPDATE > action instead, so this usage completely went under the radar for that > patch. I don't particularly like overloading the meaning of attach-device in this way, but prohibiting it may be an unexpected change in behavior for some user. However if folks agree with the change we can simply adjust the test.