| Message ID | 20100601221219.GB13961@blackpad.lan.raisama.net |
|---|---|
| State | New |
| Headers | show |
On Tue, 1 Jun 2010 19:12:19 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > 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. Makes sense, although I have no idea if doing a bdrv_close() on a bs which doesn't have an image inserted has any side effect. Basic testing works as expected, though. Anyway, I think this should go through the block queue.
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Tue, 1 Jun 2010 19:12:19 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > >> 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. > > Makes sense, although I have no idea if doing a bdrv_close() on a bs which > doesn't have an image inserted has any side effect. Should be a no-op. > Basic testing works as expected, though. > > Anyway, I think this should go through the block queue. Makes sense.
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. 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. > 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. Kevin
Kevin Wolf <kwolf@redhat.com> 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.
On Mon, Jun 07, 2010 at 02:19:28PM +0200, Markus Armbruster wrote: > Kevin Wolf <kwolf@redhat.com> writes: > > Am 02.06.2010 00:12, schrieb Eduardo Habkost: > >> 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. It does, as the whole purpose of the "eject" command is to disconnect the block device from the host backing file. Awful naming, I agree, but that's the expected semantics of the command. > > 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. Exactly. For example, this is how libvirt handles changes to disk configuration: if (disk->src) { /* [...] */ ret = qemuMonitorChangeMedia(priv->mon, driveAlias, disk->src, format); } else { ret = qemuMonitorEjectMedia(priv->mon, driveAlias); } There are existing users of the "eject" command that expect this semantics, as it is the only available way to disconnect a block device. If we want to solve the naming confusion, this could be implemented as a special case of the "change" command instead, and then the "eject" command could be deprecated.
Am 07.06.2010 14:43, schrieb Eduardo Habkost: > On Mon, Jun 07, 2010 at 02:19:28PM +0200, Markus Armbruster wrote: >> Kevin Wolf <kwolf@redhat.com> writes: >>> Am 02.06.2010 00:12, schrieb Eduardo Habkost: >>>> 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. > > It does How that? Even if the host device is still connected, but no there's no medium in it, the guest shouldn't see a medium (I mean, which medium should it see if there is none?) > as the whole purpose of the "eject" command is to disconnect > the block device from the host backing file. > > Awful naming, I agree, but that's the expected semantics of the command. If it's just meant to say "disconnect the image" it's a really bad name. Luiz, can we please get rid of it before QMP becomes stable? > If we want to solve the naming confusion, this could be implemented as a > special case of the "change" command instead, and then the "eject" > command could be deprecated. Sounds much better, though it was suggested to deprecate "change" itself, too. ;-) Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 07.06.2010 14:43, schrieb Eduardo Habkost: >> On Mon, Jun 07, 2010 at 02:19:28PM +0200, Markus Armbruster wrote: >>> Kevin Wolf <kwolf@redhat.com> writes: >>>> Am 02.06.2010 00:12, schrieb Eduardo Habkost: >>>>> 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. >> >> It does > > How that? Even if the host device is still connected, but no there's no > medium in it, the guest shouldn't see a medium (I mean, which medium > should it see if there is none?) > >> as the whole purpose of the "eject" command is to disconnect >> the block device from the host backing file. >> >> Awful naming, I agree, but that's the expected semantics of the command. > > If it's just meant to say "disconnect the image" it's a really bad name. > Luiz, can we please get rid of it before QMP becomes stable? I can create a better-named command in my blockdev series. Then we can purge "eject" from QMP. >> If we want to solve the naming confusion, this could be implemented as a >> special case of the "change" command instead, and then the "eject" >> command could be deprecated. > > Sounds much better, though it was suggested to deprecate "change" > itself, too. ;-) I intend to replace "change" in my blockdev series.
diff --git a/monitor.c b/monitor.c index 15b53b9..57c355a 100644 --- a/monitor.c +++ b/monitor.c @@ -951,20 +951,18 @@ static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data) static int eject_device(Monitor *mon, BlockDriverState *bs, int force) { - if (bdrv_is_inserted(bs)) { - if (!force) { - if (!bdrv_is_removable(bs)) { - qerror_report(QERR_DEVICE_NOT_REMOVABLE, - bdrv_get_device_name(bs)); - return -1; - } - if (bdrv_is_locked(bs)) { - qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); - return -1; - } + if (!force) { + if (!bdrv_is_removable(bs)) { + qerror_report(QERR_DEVICE_NOT_REMOVABLE, + bdrv_get_device_name(bs)); + return -1; + } + if (bdrv_is_locked(bs)) { + qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs)); + return -1; } - bdrv_close(bs); } + bdrv_close(bs); return 0; }
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. 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 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. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- monitor.c | 22 ++++++++++------------ 1 files changed, 10 insertions(+), 12 deletions(-)