Re: [Qemu-devel] [PATCH] monitor: allow device to be ejected if no disk is inserted

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 02.06.2010 00:12, schrieb Eduardo Habkost:
>> Resubmitting a patch that was submitted in December[1]. It was on the staging
>> tree but somehow it got dropped. I have rebased it to current master branch 
>> on
>> git.
>> 
>>   [1] http://article.gmane.org/gmane.comp.emulators.qemu/59813
>> 
>> --------
>> 
>> This changes the monitor eject_device() function to not check for
>> bdrv_is_inserted().
>> 
>> Example run where the bug manifests itself:
>> 
>> (output of 'info block' is stripped to include only the CD-ROM device)
>> 
>>   (qemu) info block
>>   ide1-cd0: type=cdrom removable=1 locked=0 [not inserted]
>>   (qemu) change ide1-cd0 /dev/cdrom host_cdrom
>>   (qemu) info block
>>   ide1-cd0: type=cdrom removable=1 locked=0 file=/dev/cdrom ro=1 
>> drv=host_cdrom encrypted=0
>>   (qemu) eject ide1-cd0
>>   (qemu) info block
>>   ide1-cd0: type=cdrom removable=1 locked=0 file=/dev/cdrom ro=1 
>> drv=host_cdrom encrypted=0
>> 
>>   # at this point, a disk was inserted on the host CD-ROM drive
>> 
>>   (qemu) info block
>>   ide1-cd0: type=cdrom removable=1 locked=0 file=/dev/cdrom ro=1 
>> drv=host_cdrom encrypted=0
>>   (qemu) eject ide1-cd0
>>   (qemu) info block
>>   ide1-cd0: type=cdrom removable=1 locked=0 [not inserted]
>>   (qemu)
>> 
>> The first eject command didn't work because the is_inserted() check
>> failed.
>
> But does it really make a difference? The guest should not see a medium
> before and it should not see one afterwards.
>
>> I have no clue why the code had the is_inserted() check, as it doesn't matter
>> if there is a disk present at the host drive, when the user wants the virtual
>> device to be disconnected from the host device.
>
> The question is what the semantics of the eject monitor command is
> supposed to be. I for one would have expected that it means that if
> there was a medium inserted in the virtual CD-ROM drive, it won't be
> there afterwards. I wouldn't have expected the connection to the host
> device to be affected.

But that's what it does: remove the media from the block device host
part, leaving the device empty.

>From the guest's point of view, that looks like the media was ejected.

> Actually, what I would have expected is not calling bdrv_close(), but
> calling bdrv_eject() and possibly doing something with the device state
> to reflect that. If the VM gets a real CD-ROM passed through, eject for
> the virtual device should just mean eject for the real device.

That's a different "eject".  As so often, QEMU uses the same name in
several ways, just to keep us on our toes.

bdrv_eject() lets guest devices ask the block driver to eject media.
This is useful mainly for device pass through: when guest OS tells the
virtual hardware to eject, the device model calls bdrv_eject(), which
calls block driver method bdrv_eject(), if the block driver implements
it.  Pass through drivers such as "host_cdrom" do, and eject the host
cdrom.

>> The is_inserted() check has another side effect: a memory leak if the 
>> "change"
>> command is used multiple times, as do_change() calls eject_device() before
>> re-opening the block device, but bdrv_close() is never called.
>
> In the context of do_change the desired semantics is probably a
> different one, I agree. It probably shouldn't call do_eject.

do_eject() and do_change_block() are closely related.  The former
removes the media from the block device host part.  The latter
additionally inserts new media.