[Qemu-devel] [PATCH 0/2] integrate eject/uneject fixes

[Qemu-devel] [PATCH 0/2] integrate eject/uneject fixes

[Eduardo Habkost]

This series contains two patches that were submitted previously, but conflict
with each other. This series contains both to make sure they are applied on the
right order and that there are no mismerges.

This should fix a mismerge that is present on the current staging tree (a
closing bracket is missing on eject_device()).

Naphtali's patch should have included the bdrv_forget_fname() call inside the
bdrv_is_inserted() if block. But instead of fixing it, it can be simply applied
after my patch.

Eduardo Habkost (1):
  monitor: allow device to be ejected if no disk is inserted

Naphtali Sprei (1):
  Fix for cdrom un-eject

 block.c       |   24 ++++++++++++++++++++----
 block.h       |    1 +
 block_int.h   |    1 +
 hw/ide/core.c |   35 +++++++++++++++++++++++++----------
 monitor.c     |   21 ++++++++++-----------
 5 files changed, 57 insertions(+), 25 deletions(-)

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

[Eduardo Habkost]

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 0.11.50 monitor - type 'help' for more information
  (qemu) info block
  ide1-cd0: type=cdrom removable=1 locked=0 [not inserted]
  (qemu) change ide1-cd0 /mnt/common/images/smalldisk1.img
  (qemu) info block
  ide1-cd0: type=cdrom removable=1 locked=0 file=/mnt/common/images/smalldisk1.img ro=0 drv=raw encrypted=0
  (qemu) eject ide1-cd0
  (qemu) info block
  ide1-cd0: type=cdrom removable=1 locked=0 [not inserted]

When using a file, eject works as expected. But when using a host cdrom device:

  (qemu) change ide1-cd0 /dev/cdrom
  (qemu) info block
  ide1-cd0: type=cdrom removable=1 locked=0 file=/dev/cdrom ro=0 drv=host_cdrom encrypted=0
  (qemu) eject ide1-cd0
  (qemu) info block
  ide1-cd0: type=cdrom removable=1 locked=0 file=/dev/cdrom ro=0 drv=host_cdrom encrypted=0

Eject didn't work because the is_inserted() check fails.

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() 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 |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/monitor.c b/monitor.c
index 5bf32f0..d882c51 100644
--- a/monitor.c
+++ b/monitor.c
@@ -741,19 +741,17 @@ static void 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)) {
-                monitor_printf(mon, "device is not removable\n");
-                return -1;
-            }
-            if (bdrv_is_locked(bs)) {
-                monitor_printf(mon, "device is locked\n");
-                return -1;
-            }
+    if (!force) {
+        if (!bdrv_is_removable(bs)) {
+            monitor_printf(mon, "device is not removable\n");
+            return -1;
+        }
+        if (bdrv_is_locked(bs)) {
+            monitor_printf(mon, "device is locked\n");
+            return -1;
         }
-        bdrv_close(bs);
     }
+    bdrv_close(bs);
     return 0;
 }
 
-- 
1.6.3.rc4.29.g8146

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

[Eduardo Habkost]

Rebasing previous patch to latest staging tree. For the master branch and
stable-0.12.

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 0.11.50 monitor - type 'help' for more information
  (qemu) info block
  ide1-cd0: type=cdrom removable=1 locked=0 [not inserted]
  (qemu) change ide1-cd0 /mnt/common/images/smalldisk1.img
  (qemu) info block
  ide1-cd0: type=cdrom removable=1 locked=0 file=/mnt/common/images/smalldisk1.img ro=0 drv=raw encrypted=0
  (qemu) eject ide1-cd0
  (qemu) info block
  ide1-cd0: type=cdrom removable=1 locked=0 [not inserted]

When using a file, eject works as expected. But when using a host cdrom device:

  (qemu) change ide1-cd0 /dev/cdrom
  (qemu) info block
  ide1-cd0: type=cdrom removable=1 locked=0 file=/dev/cdrom ro=0 drv=host_cdrom encrypted=0
  (qemu) eject ide1-cd0
  (qemu) info block
  ide1-cd0: type=cdrom removable=1 locked=0 file=/dev/cdrom ro=0 drv=host_cdrom encrypted=0

Eject didn't work because the is_inserted() check fails.

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() 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(-)

diff --git a/monitor.c b/monitor.c
index 96f7876..97fbe25 100644
--- a/monitor.c
+++ b/monitor.c
@@ -846,20 +846,18 @@ static void 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)) {
-                qemu_error_new(QERR_DEVICE_NOT_REMOVABLE,
-                               bdrv_get_device_name(bs));
-                return -1;
-            }
-            if (bdrv_is_locked(bs)) {
-                qemu_error_new(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
-                return -1;
-            }
+    if (!force) {
+        if (!bdrv_is_removable(bs)) {
+            qemu_error_new(QERR_DEVICE_NOT_REMOVABLE,
+                           bdrv_get_device_name(bs));
+            return -1;
+        }
+        if (bdrv_is_locked(bs)) {
+            qemu_error_new(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
+            return -1;
         }
-        bdrv_close(bs);
     }
+    bdrv_close(bs);
     return 0;
 }
 
-- 
1.6.3.rc4.29.g8146
-- 
Eduardo

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

[Luiz Capitulino]

On Fri, 18 Dec 2009 17:07:43 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> When using a file, eject works as expected. But when using a host cdrom device:
> 
>   (qemu) change ide1-cd0 /dev/cdrom
>   (qemu) info block
>   ide1-cd0: type=cdrom removable=1 locked=0 file=/dev/cdrom ro=0 drv=host_cdrom encrypted=0
>   (qemu) eject ide1-cd0
>   (qemu) info block
>   ide1-cd0: type=cdrom removable=1 locked=0 file=/dev/cdrom ro=0 drv=host_cdrom encrypted=0

 Sorry for only taking a look at this now, but it works for me.

 Couldn't this be a bug somewhere else in the block layer?

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

[Eduardo Habkost]

On Fri, Dec 18, 2009 at 06:15:27PM -0200, Luiz Capitulino wrote:
> On Fri, 18 Dec 2009 17:07:43 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > When using a file, eject works as expected. But when using a host cdrom device:
> > 
> >   (qemu) change ide1-cd0 /dev/cdrom
> >   (qemu) info block
> >   ide1-cd0: type=cdrom removable=1 locked=0 file=/dev/cdrom ro=0 drv=host_cdrom encrypted=0
> >   (qemu) eject ide1-cd0
> >   (qemu) info block
> >   ide1-cd0: type=cdrom removable=1 locked=0 file=/dev/cdrom ro=0 drv=host_cdrom encrypted=0
> 
>  Sorry for only taking a look at this now, but it works for me.

Do you have a disk on your CD-ROM drive?

-- 
Eduardo

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

[Luiz Capitulino]

On Fri, 18 Dec 2009 18:22:41 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Dec 18, 2009 at 06:15:27PM -0200, Luiz Capitulino wrote:
> > On Fri, 18 Dec 2009 17:07:43 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > When using a file, eject works as expected. But when using a host cdrom device:
> > > 
> > >   (qemu) change ide1-cd0 /dev/cdrom
> > >   (qemu) info block
> > >   ide1-cd0: type=cdrom removable=1 locked=0 file=/dev/cdrom ro=0 drv=host_cdrom encrypted=0
> > >   (qemu) eject ide1-cd0
> > >   (qemu) info block
> > >   ide1-cd0: type=cdrom removable=1 locked=0 file=/dev/cdrom ro=0 drv=host_cdrom encrypted=0
> > 
> >  Sorry for only taking a look at this now, but it works for me.
> 
> Do you have a disk on your CD-ROM drive?

 Yeah, I had.. Quickly tested you patch and it fixed the problem to me.

[Qemu-devel] [PATCH 2/2] Fix for cdrom un-eject

[Eduardo Habkost]

From: Naphtali Sprei <nsprei@redhat.com>

When guest un-eject a cdrom, re-insert the cdrom image (re-open the drive's file).

Also, related changes for the un-eject:
o  enter UNIT ATTENTION state only on change/insert media, not upon removal
o  minor change in packet command abort when in UNIT ATTENTION state (as per spec)
o  enter UNIT ATTENTION state upon reset (as per spec)
o  always print the file-name in info block command (if applicable)

Relevant Spec: Mt. Fuji Commands for Multimedia Devices Version 7: ftp://ftp.seagate.com/sff/INF-8090.PDF

[ehabkost: solved conficts on monitor.c eject_device()]

Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 block.c       |   24 ++++++++++++++++++++----
 block.h       |    1 +
 block_int.h   |    1 +
 hw/ide/core.c |   35 +++++++++++++++++++++++++----------
 monitor.c     |    1 +
 5 files changed, 48 insertions(+), 14 deletions(-)

diff --git a/block.c b/block.c
index 6fdabff..1801569 100644
--- a/block.c
+++ b/block.c
@@ -463,6 +463,7 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
         ret = drv->bdrv_open(bs, filename, open_flags & ~BDRV_O_RDWR);
         bs->read_only = 1;
     }
+    bs->open_flags = flags;
     if (ret < 0) {
         qemu_free(bs->opaque);
         bs->opaque = NULL;
@@ -1155,9 +1156,11 @@ void bdrv_info(Monitor *mon)
         if (bs->removable) {
             monitor_printf(mon, " locked=%d", bs->locked);
         }
-        if (bs->drv) {
+        if (bs->filename[0]) {
             monitor_printf(mon, " file=");
             monitor_print_filename(mon, bs->filename);
+        }
+        if (bs->drv) {
             if (bs->backing_file[0] != '\0') {
                 monitor_printf(mon, " backing_file=");
                 monitor_print_filename(mon, bs->backing_file);
@@ -1907,14 +1910,27 @@ int bdrv_eject(BlockDriverState *bs, int eject_flag)
         ret = drv->bdrv_eject(bs, eject_flag);
     }
     if (ret == -ENOTSUP) {
-        if (eject_flag)
+        if (eject_flag) {
             bdrv_close(bs);
-        ret = 0;
+            ret = 0;
+        } else { /* re-insert media */
+            if (bs->filename[0]) {
+                ret = bdrv_open(bs, bs->filename, bs->open_flags);
+            } else {
+                ret = 0;
+            }
+        }
     }
-
+    
     return ret;
 }
 
+
+void bdrv_forget_fname(BlockDriverState *bs)
+{
+    bs->filename[0] = '\0';
+}
+
 int bdrv_is_locked(BlockDriverState *bs)
 {
     return bs->locked;
diff --git a/block.h b/block.h
index 2d4f066..8fc10d2 100644
--- a/block.h
+++ b/block.h
@@ -147,6 +147,7 @@ int bdrv_media_changed(BlockDriverState *bs);
 int bdrv_is_locked(BlockDriverState *bs);
 void bdrv_set_locked(BlockDriverState *bs, int locked);
 int bdrv_eject(BlockDriverState *bs, int eject_flag);
+void bdrv_forget_fname(BlockDriverState *bs);
 void bdrv_set_change_cb(BlockDriverState *bs,
                         void (*change_cb)(void *opaque), void *opaque);
 void bdrv_get_format(BlockDriverState *bs, char *buf, int buf_size);
diff --git a/block_int.h b/block_int.h
index 7ebe926..1ecf7bf 100644
--- a/block_int.h
+++ b/block_int.h
@@ -137,6 +137,7 @@ struct BlockDriverState {
     void *opaque;
 
     char filename[1024];
+    int  open_flags;
     char backing_file[1024]; /* if non zero, the image is a diff of
                                 this file image */
     char backing_format[16]; /* if non-zero and backing_file exists */
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 7b1ff8f..5631898 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -35,6 +35,20 @@
 
 #include <hw/ide/internal.h>
 
+
+static int is_pcommand_abort_on_check(uint8_t pcmd) {
+    int ret = 1;
+
+    if (pcmd == GPCMD_GET_CONFIGURATION ||
+        pcmd == GPCMD_REQUEST_SENSE ||
+	pcmd == GPCMD_INQUIRY) {
+        ret = 0;
+    }
+    return ret;
+}
+
+
+
 static int smart_attributes[][5] = {
     /* id,  flags, val, wrst, thrsh */
     { 0x01, 0x03, 0x64, 0x64, 0x06}, /* raw read */
@@ -1206,12 +1220,11 @@ static void ide_atapi_cmd(IDEState *s)
     }
 #endif
     /* If there's a UNIT_ATTENTION condition pending, only
-       REQUEST_SENSE and INQUIRY commands are allowed to complete. */
+       REQUEST_SENSE and INQUIRY commands (and some more) are allowed to complete. */
     if (s->sense_key == SENSE_UNIT_ATTENTION &&
-	s->io_buffer[0] != GPCMD_REQUEST_SENSE &&
-	s->io_buffer[0] != GPCMD_INQUIRY) {
-	ide_atapi_cmd_check_status(s);
-	return;
+        is_pcommand_abort_on_check(s->io_buffer[0])) {
+        ide_atapi_cmd_check_status(s);
+        return;
     }
     switch(s->io_buffer[0]) {
     case GPCMD_TEST_UNIT_READY:
@@ -1676,9 +1689,11 @@ static void cdrom_change_cb(void *opaque)
     bdrv_get_geometry(s->bs, &nb_sectors);
     s->nb_sectors = nb_sectors;
 
-    s->sense_key = SENSE_UNIT_ATTENTION;
-    s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
-    s->cdrom_changed = 1;
+    if (nb_sectors > 0) { /* UNIT_ATTENTION only for insertion/change, not removal */
+        s->sense_key = SENSE_UNIT_ATTENTION;
+        s->asc = ASC_MEDIUM_MAY_HAVE_CHANGED;
+        s->cdrom_changed = 1;
+    }
     ide_set_irq(s->bus);
 }
 
@@ -2530,8 +2545,8 @@ static void ide_reset(IDEState *s)
     s->lba48 = 0;
 
     /* ATAPI specific */
-    s->sense_key = 0;
-    s->asc = 0;
+    s->sense_key = SENSE_UNIT_ATTENTION;
+    s->asc = 0x29; /*  POWER ON, RESET, OR BUS DEVICE RESET OCCURRED */
     s->cdrom_changed = 0;
     s->packet_transfer_size = 0;
     s->elementary_transfer_size = 0;
diff --git a/monitor.c b/monitor.c
index d882c51..6135ead 100644
--- a/monitor.c
+++ b/monitor.c
@@ -752,6 +752,7 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
         }
     }
     bdrv_close(bs);
+    bdrv_forget_fname(bs); /* "manual" eject forgets the file-name */
     return 0;
 }
 
-- 
1.6.3.rc4.29.g8146

Re: [Qemu-devel] [PATCH 2/2] Fix for cdrom un-eject

[Eduardo Habkost]

Excerpts from Eduardo Habkost's message of Thu Dec 03 14:33:21 -0200 2009:
> From: Naphtali Sprei <nsprei@redhat.com>
> 
> When guest un-eject a cdrom, re-insert the cdrom image (re-open the drive's
> file).
> 
> Also, related changes for the un-eject:
> o  enter UNIT ATTENTION state only on change/insert media, not upon removal
> o  minor change in packet command abort when in UNIT ATTENTION state (as per
> spec)
> o  enter UNIT ATTENTION state upon reset (as per spec)
> o  always print the file-name in info block command (if applicable)
> 
> Relevant Spec: Mt. Fuji Commands for Multimedia Devices Version 7:
> ftp://ftp.seagate.com/sff/INF-8090.PDF
> 
> [ehabkost: solved conficts on monitor.c eject_device()]
> 
> Signed-off-by: Naphtali Sprei <nsprei@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

This patch needs to be rebaed to the new monitor printing code.
Naphtali, do you plan to do it?
-- 
Eduardo