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

[Pavel Hrdina]

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