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

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.