Re: [libvirt] [PATCH] virsh: Correctly detect inserted media in change-media command

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);