On Wed, Apr 10, 2019 at 01:33:11PM -0600, Jim Fehlig wrote:
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>
> > > > 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.
We must not break API back-compatibility in this way, so must fix the
regression.
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 :|