Patchwork monitor: allow device to be ejected if no disk is inserted

login
register
mail settings
Submitter Eduardo Habkost
Date June 1, 2010, 10:12 p.m.
Message ID <20100601221219.GB13961@blackpad.lan.raisama.net>
Download mbox | patch
Permalink /patch/54332/
State New
Headers show

Comments

Eduardo Habkost - June 1, 2010, 10:12 p.m.
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(-)
Luiz Capitulino - June 4, 2010, 6:01 p.m.
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.
Markus Armbruster - June 7, 2010, 6:57 a.m.
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.
Kevin Wolf - June 7, 2010, 11:18 a.m.
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
Markus Armbruster - June 7, 2010, 12:19 p.m.
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.
Eduardo Habkost - June 7, 2010, 12:43 p.m.
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.
Kevin Wolf - June 7, 2010, 12:53 p.m.
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
Markus Armbruster - June 7, 2010, 1:18 p.m.
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.

Patch

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;
 }