On Mon, Jan 25, 2016 at 04:03:44PM +0100, Michal Privoznik wrote: > On 25.01.2016 15:46, Pavel Hrdina wrote: > > On Mon, Jan 25, 2016 at 03:21:14PM +0100, Michal Privoznik wrote: > >> On 25.01.2016 13:45, Pavel Hrdina wrote: > >>> On Wed, Jan 13, 2016 at 05:39:10PM +0100, Michal Privoznik wrote: > >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1250331 > >>>> > >>>> It all works like this. The change-media command dumps domain > >>>> XML, finds the corresponding cdrom device we want to change media > >>>> in and returns it in the xmlNodePtr form. This way we don't have > >>>> to bother with keeping all the subelements or attributes that we > >>>> don't care about in the XML that is fed back to libvirt for the > >>>> update API. > >>>> > >>>> Now, the problem is we try to be clever here and detect if disk > >>>> already has a source (indicated by <source/> subelement). > >>>> However, bare fact that the element is there does not mean disk > >>>> has source. The element has some attributes and only if @file or > >>>> @dev is within them disk has source. Any other attribute is > >>>> meaningless for our purpose now. Make our clever check better. > >>> > >>> That's not true, what about disk type='dir|volume|network'? Those could be also > >>> used as cdrom or floppy. The patch looks good, but extend it to detect all > >>> possible disk types. > >>> > >> > >> Well, the code doesn't know how to deal with those types anyway. For all > >> types you've pointed out we will change the disk type to file. So in the > >> end from a type='dir|volume|network' disk we will create type='file'. I > >> see no point in producing the following XML then: > >> > >> <disk type='file' device='cdrom'> > >> <source file='example.host.com'/> > >> ... > >> </disk> > >> > >> But that's a bigger bug to fix. Meanwhile I think we can use my > >> (incomplete?) fix. > >> > >> Michal > > > > No, I'm not talking about the XML that we will create. Let's say, that you have > > a domain and there is this device: > > > > <disk type='dir' device='cdrom'> > > <driver name='qemu'/> > > <source dir='/tmp/tmp-cdrom'/> > > <target dev='sdc' bus='sata'/> > > <readonly/> > > <address type='drive' controller='0' bus='0' target='0' unit='2'/> > > </disk> > > > > Your patch would simply ignore the dir source and assumed that there is no media > > inserted and create a new definition. It would be regression from current > > behavior. It doesn't meter whether the new cdrom would be block or file, you > > need to check the existing device and consider all possible source types to > > detect whether there is a media inserted or not. > > > > A-ha, now I see the problem. Would squashing this in help? > > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c > index f023e3d..dd41260 100644 > --- a/tools/virsh-domain.c > +++ b/tools/virsh-domain.c > @@ -11112,8 +11112,11 @@ virshUpdateDiskXML(xmlNodePtr disk_node, > } > > if (source) { > - if (!(source_path = virXMLPropString(source, "file"))) > - source_path = virXMLPropString(source, "dev"); > + if (!(source_path = virXMLPropString(source, "file")) && > + !(source_path = virXMLPropString(source, "dev")) && > + !(source_path = virXMLPropString(source, "dir")) && > + !(source_path = virXMLPropString(source, "pool"))) > + source_path = virXMLPropString(source, "name"); > > if (source_path && type == VIRSH_UPDATE_DISK_XML_INSERT) { > vshError(NULL, _("The disk device '%s' already has media"), target); ACK with that :) -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list