[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] monitor: allow device to be ejected if no disk
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH] monitor: allow device to be ejected if no disk is inserted |
Date: |
Mon, 07 Jun 2010 14:19:28 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
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.