[Qemu-devel] [PATCH 0/4] blockdev: Fix 'change' for slot devices

[Qemu-devel] [PATCH 0/4] blockdev: Fix 'change' for slot devices

[Max Reitz]

The series "BlockBackend and media" intended all block devices with
removable media to implement a tray model; if the devices does not have
a tray, it should emulate one.

While this may make sense from a technical perspective (blockdev-*-tray
are guest device controlling operations, invoking
blk_dev_change_media_cb(); blockdev-*-medium are operations concerning
the block layer, controlling the BB-BDS link), it is (probably)
unintuitive to users, and it requires said implementation of an emulated
tray for each of the slot devices (floppy disk drives and SD card
readers).

We can get rid of those virtual trays by special-casing tray-less
devices in blockdev-*-tray (those operations are no-ops there) and in
blockdev-*-medium (those operations then have to invoke
blk_dev_change_media_cb()). With this change, changing the medium
inserted into a slot device will no longer emit TRAY_MOVED events (which
seems like a bugfix to me, because slot devices actually do not have
trays).


Patches 1 and 2 are CC'd to qemu-stable because they fix 'change' for SD
card readers. Patch 3 does not fix it for floppy disk drives, it just
changes the behavior when used on them (no TRAY_MOVED events, no
tray_open status, and blockdev-{open,close}-tray are optional), which is
why it is not CC'd to qemu-stable; and patch 4 is more of a change in
behavior than a stable-worthy fix.


Max Reitz (4):
  block: Add blk_dev_has_tray()
  blockdev: Fix 'change' for slot devices
  Revert "hw/block/fdc: Implement tray status"
  block/qapi: Emit tray_open only if there is a tray

 block/block-backend.c      |  10 +++-
 block/qapi.c               |   2 +-
 blockdev.c                 |  27 ++++++++++-
 hw/block/fdc.c             |  20 ++------
 include/block/block_int.h  |   1 +
 qapi/block-core.json       |   3 +-
 tests/fdc-test.c           |   2 -
 tests/qemu-iotests/067.out |   4 --
 tests/qemu-iotests/118     | 117 ++++++++++++++-------------------------------
 9 files changed, 77 insertions(+), 109 deletions(-)

-- 
2.7.0

[Qemu-devel] [PATCH 1/4] block: Add blk_dev_has_tray()

[Max Reitz]

Pull out the check whether a block device has a tray from
blk_dev_is_tray_open() into an own function so both attributes (whether
there is a tray vs. whether that tray is open) can be queried
independently.

Cc: qemu-stable <qemu-stable@nongnu.org>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c     | 10 +++++++++-
 include/block/block_int.h |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index f41d326..60f9588 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -458,6 +458,14 @@ bool blk_dev_has_removable_media(BlockBackend *blk)
 }
 
 /*
+ * Does @blk's attached device model have a tray?
+ */
+bool blk_dev_has_tray(BlockBackend *blk)
+{
+    return blk->dev_ops && blk->dev_ops->is_tray_open;
+}
+
+/*
  * Notify @blk's attached device model of a media eject request.
  * If @force is true, the medium is about to be yanked out forcefully.
  */
@@ -473,7 +481,7 @@ void blk_dev_eject_request(BlockBackend *blk, bool force)
  */
 bool blk_dev_is_tray_open(BlockBackend *blk)
 {
-    if (blk->dev_ops && blk->dev_ops->is_tray_open) {
+    if (blk_dev_has_tray(blk)) {
         return blk->dev_ops->is_tray_open(blk->dev_opaque);
     }
     return false;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 256609d..14bf022 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -694,6 +694,7 @@ void blk_set_bs(BlockBackend *blk, BlockDriverState *bs);
 
 void blk_dev_change_media_cb(BlockBackend *blk, bool load);
 bool blk_dev_has_removable_media(BlockBackend *blk);
+bool blk_dev_has_tray(BlockBackend *blk);
 void blk_dev_eject_request(BlockBackend *blk, bool force);
 bool blk_dev_is_tray_open(BlockBackend *blk);
 bool blk_dev_is_medium_locked(BlockBackend *blk);
-- 
2.7.0

[Qemu-devel] [PATCH 2/4] blockdev: Fix 'change' for slot devices

[Max Reitz]

'change' and related operations did not work when used on guest devices
featuring removable media but no actual tray, because
blk_dev_is_tray_open() always returned false for them and the
blockdev-{insert,remove}-medium commands required it to return true.

Fix this by making blockdev-{insert,remove}-medium work on tray-less
devices. Also, blockdev-{open,close}-tray are now explicitly no-ops when
invoked on such devices, and blk_dev_change_media_cb() is instead
called by blockdev-{insert,remove}-medium (for tray-less devices only).

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-stable <qemu-stable@nongnu.org>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c           | 27 +++++++++++++++++++++++++--
 qapi/block-core.json |  3 +--
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 2df0c6d..f053be6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2305,6 +2305,11 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
         return;
     }
 
+    if (!blk_dev_has_tray(blk)) {
+        /* Ignore this command on tray-less devices */
+        return;
+    }
+
     if (blk_dev_is_tray_open(blk)) {
         return;
     }
@@ -2335,6 +2340,11 @@ void qmp_blockdev_close_tray(const char *device, Error **errp)
         return;
     }
 
+    if (!blk_dev_has_tray(blk)) {
+        /* Ignore this command on tray-less devices */
+        return;
+    }
+
     if (!blk_dev_is_tray_open(blk)) {
         return;
     }
@@ -2364,7 +2374,7 @@ void qmp_x_blockdev_remove_medium(const char *device, Error **errp)
         return;
     }
 
-    if (has_device && !blk_dev_is_tray_open(blk)) {
+    if (has_device && blk_dev_has_tray(blk) && !blk_dev_is_tray_open(blk)) {
         error_setg(errp, "Tray of device '%s' is not open", device);
         return;
     }
@@ -2381,6 +2391,12 @@ void qmp_x_blockdev_remove_medium(const char *device, Error **errp)
         goto out;
     }
 
+    if (!blk_dev_has_tray(blk)) {
+        /* For tray-less devices, blockdev-open-tray is a no-op (or may not be
+         * called at all); therefore, the medium needs to be ejected here */
+        blk_dev_change_media_cb(blk, false);
+    }
+
     /* This follows the convention established by bdrv_make_anon() */
     if (bs->device_list.tqe_prev) {
         QTAILQ_REMOVE(&bdrv_states, bs, device_list);
@@ -2414,7 +2430,7 @@ static void qmp_blockdev_insert_anon_medium(const char *device,
         return;
     }
 
-    if (has_device && !blk_dev_is_tray_open(blk)) {
+    if (has_device && blk_dev_has_tray(blk) && !blk_dev_is_tray_open(blk)) {
         error_setg(errp, "Tray of device '%s' is not open", device);
         return;
     }
@@ -2424,6 +2440,13 @@ static void qmp_blockdev_insert_anon_medium(const char *device,
         return;
     }
 
+    if (!blk_dev_has_tray(blk)) {
+        /* For tray-less devices, blockdev-close-tray is a no-op (or may not be
+         * called at all); therefore, the medium needs to be pushed into the
+         * slot here */
+        blk_dev_change_media_cb(blk, true);
+    }
+
     blk_insert_bs(blk, bs);
 
     QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0a915ed..40239bf 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2098,8 +2098,7 @@
 #   respond to the eject request
 # - if the BlockBackend denoted by @device does not have a guest device attached
 #   to it
-# - if the guest device does not have an actual tray and is empty, for instance
-#   for floppy disk drives
+# - if the guest device does not have an actual tray
 #
 # @device: block device name
 #
-- 
2.7.0

[Qemu-devel] [PATCH 3/4] Revert "hw/block/fdc: Implement tray status"

[Max Reitz]

This reverts the changes commit
2e1280e8ff95b3145bc6262accc9d447718e5318 applied to hw/block/fdc.c.

That commit changed tests/fdc-test.c, too, because after it, one less
TRAY_MOVED event would be emitted when executing 'change' on an empty
drive. However, now, no TRAY_MOVED events will be emitted at all, and
the tray_open status returned by query-block will always be false,
necessitating (different) changes to tests/fdc-test.c and iotest 118,
which is why this patch is not a pure revert of said commit.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 hw/block/fdc.c         |  20 ++-------
 tests/fdc-test.c       |   2 -
 tests/qemu-iotests/118 | 117 +++++++++++++++----------------------------------
 3 files changed, 40 insertions(+), 99 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 4292ece..6686a72 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -192,8 +192,6 @@ typedef struct FDrive {
     uint8_t ro;               /* Is read-only           */
     uint8_t media_changed;    /* Is media changed       */
     uint8_t media_rate;       /* Data rate of medium    */
-
-    bool media_inserted;      /* Is there a medium in the tray */
 } FDrive;
 
 static void fd_init(FDrive *drv)
@@ -263,7 +261,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
 #endif
         drv->head = head;
         if (drv->track != track) {
-            if (drv->media_inserted) {
+            if (drv->blk != NULL && blk_is_inserted(drv->blk)) {
                 drv->media_changed = 0;
             }
             ret = 1;
@@ -272,7 +270,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
         drv->sect = sect;
     }
 
-    if (!drv->media_inserted) {
+    if (drv->blk == NULL || !blk_is_inserted(drv->blk)) {
         ret = 2;
     }
 
@@ -298,7 +296,7 @@ static void fd_revalidate(FDrive *drv)
         ro = blk_is_read_only(drv->blk);
         pick_geometry(drv->blk, &nb_heads, &max_track,
                       &last_sect, drv->drive, &drive, &rate);
-        if (!drv->media_inserted) {
+        if (!blk_is_inserted(drv->blk)) {
             FLOPPY_DPRINTF("No disk in drive\n");
         } else {
             FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n", nb_heads,
@@ -694,7 +692,7 @@ static bool fdrive_media_changed_needed(void *opaque)
 {
     FDrive *drive = opaque;
 
-    return (drive->media_inserted && drive->media_changed != 1);
+    return (drive->blk != NULL && drive->media_changed != 1);
 }
 
 static const VMStateDescription vmstate_fdrive_media_changed = {
@@ -2186,21 +2184,12 @@ static void fdctrl_change_cb(void *opaque, bool load)
 {
     FDrive *drive = opaque;
 
-    drive->media_inserted = load && drive->blk && blk_is_inserted(drive->blk);
-
     drive->media_changed = 1;
     fd_revalidate(drive);
 }
 
-static bool fdctrl_is_tray_open(void *opaque)
-{
-    FDrive *drive = opaque;
-    return !drive->media_inserted;
-}
-
 static const BlockDevOps fdctrl_block_ops = {
     .change_media_cb = fdctrl_change_cb,
-    .is_tray_open = fdctrl_is_tray_open,
 };
 
 /* Init functions */
@@ -2228,7 +2217,6 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp)
         fdctrl_change_cb(drive, 0);
         if (drive->blk) {
             blk_set_dev_ops(drive->blk, &fdctrl_block_ops, drive);
-            drive->media_inserted = blk_is_inserted(drive->blk);
         }
     }
 }
diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index b5a4696..7f0e215 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -304,7 +304,6 @@ static void test_media_insert(void)
     qmp_discard_response("{'execute':'change', 'arguments':{"
                          " 'device':'floppy0', 'target': %s, 'arg': 'raw' }}",
                          test_image);
-    qmp_discard_response(""); /* ignore event (open -> close) */
 
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
@@ -335,7 +334,6 @@ static void test_media_change(void)
      * reset the bit. */
     qmp_discard_response("{'execute':'eject', 'arguments':{"
                          " 'device':'floppy0' }}");
-    qmp_discard_response(""); /* ignore event */
 
     dir = inb(FLOPPY_BASE + reg_dir);
     assert_bit_set(dir, DSKCHG);
diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index 114d0e2..7caa38c 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -42,6 +42,9 @@ class ChangeBaseClass(iotests.QMPTestCase):
                     self.has_opened = True
 
     def wait_for_open(self):
+        if not self.has_real_tray:
+            return
+
         timeout = time.clock() + 3
         while not self.has_opened and time.clock() < timeout:
             self.process_events()
@@ -49,6 +52,9 @@ class ChangeBaseClass(iotests.QMPTestCase):
             self.fail('Timeout while waiting for the tray to open')
 
     def wait_for_close(self):
+        if not self.has_real_tray:
+            return
+
         timeout = time.clock() + 3
         while not self.has_closed and time.clock() < timeout:
             self.process_events()
@@ -65,7 +71,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.wait_for_close()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
+        if self.has_real_tray:
+            self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
     def test_blockdev_change_medium(self):
@@ -78,7 +85,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.wait_for_close()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
+        if self.has_real_tray:
+            self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
     def test_eject(self):
@@ -88,7 +96,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.wait_for_open()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', True)
+        if self.has_real_tray:
+            self.assert_qmp(result, 'return[0]/tray_open', True)
         self.assert_qmp_absent(result, 'return[0]/inserted')
 
     def test_tray_eject_change(self):
@@ -98,7 +107,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.wait_for_open()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', True)
+        if self.has_real_tray:
+            self.assert_qmp(result, 'return[0]/tray_open', True)
         self.assert_qmp_absent(result, 'return[0]/inserted')
 
         result = self.vm.qmp('blockdev-change-medium', device='drive0',
@@ -109,7 +119,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.wait_for_close()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
+        if self.has_real_tray:
+            self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
     def test_tray_open_close(self):
@@ -119,7 +130,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.wait_for_open()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', True)
+        if self.has_real_tray:
+            self.assert_qmp(result, 'return[0]/tray_open', True)
         if self.was_empty == True:
             self.assert_qmp_absent(result, 'return[0]/inserted')
         else:
@@ -132,10 +144,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
             self.wait_for_close()
 
         result = self.vm.qmp('query-block')
-        if self.has_real_tray or not self.was_empty:
+        if self.has_real_tray:
             self.assert_qmp(result, 'return[0]/tray_open', False)
-        else:
-            self.assert_qmp(result, 'return[0]/tray_open', True)
         if self.was_empty == True:
             self.assert_qmp_absent(result, 'return[0]/inserted')
         else:
@@ -148,20 +158,18 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.wait_for_open()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', True)
+        if self.has_real_tray:
+            self.assert_qmp(result, 'return[0]/tray_open', True)
         self.assert_qmp_absent(result, 'return[0]/inserted')
 
         result = self.vm.qmp('blockdev-close-tray', device='drive0')
         self.assert_qmp(result, 'return', {})
 
-        if self.has_real_tray:
-            self.wait_for_close()
+        self.wait_for_close()
 
         result = self.vm.qmp('query-block')
         if self.has_real_tray:
             self.assert_qmp(result, 'return[0]/tray_open', False)
-        else:
-            self.assert_qmp(result, 'return[0]/tray_open', True)
         self.assert_qmp_absent(result, 'return[0]/inserted')
 
     def test_tray_open_change(self):
@@ -171,7 +179,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.wait_for_open()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', True)
+        if self.has_real_tray:
+            self.assert_qmp(result, 'return[0]/tray_open', True)
         if self.was_empty == True:
             self.assert_qmp_absent(result, 'return[0]/inserted')
         else:
@@ -185,7 +194,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.wait_for_close()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
+        if self.has_real_tray:
+            self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
     def test_cycle(self):
@@ -202,7 +212,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.wait_for_open()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', True)
+        if self.has_real_tray:
+            self.assert_qmp(result, 'return[0]/tray_open', True)
         if self.was_empty == True:
             self.assert_qmp_absent(result, 'return[0]/inserted')
         else:
@@ -212,7 +223,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', True)
+        if self.has_real_tray:
+            self.assert_qmp(result, 'return[0]/tray_open', True)
         self.assert_qmp_absent(result, 'return[0]/inserted')
 
         result = self.vm.qmp('x-blockdev-insert-medium', device='drive0',
@@ -220,7 +232,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', True)
+        if self.has_real_tray:
+            self.assert_qmp(result, 'return[0]/tray_open', True)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
         result = self.vm.qmp('blockdev-close-tray', device='drive0')
@@ -229,7 +242,8 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.wait_for_close()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
+        if self.has_real_tray:
+            self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
     def test_close_on_closed(self):
@@ -239,16 +253,14 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
         self.assertEquals(self.vm.get_qmp_events(wait=False), [])
 
     def test_remove_on_closed(self):
-        if self.has_opened:
-            # Empty floppy drive
+        if not self.has_real_tray:
             return
 
         result = self.vm.qmp('x-blockdev-remove-medium', device='drive0')
         self.assert_qmp(result, 'error/class', 'GenericError')
 
     def test_insert_on_closed(self):
-        if self.has_opened:
-            # Empty floppy drive
+        if not self.has_real_tray:
             return
 
         result = self.vm.qmp('blockdev-add',
@@ -366,7 +378,6 @@ class TestChangeReadOnly(ChangeBaseClass):
         self.vm.launch()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', True)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
@@ -376,11 +387,7 @@ class TestChangeReadOnly(ChangeBaseClass):
                                                        read_only_mode='retain')
         self.assert_qmp(result, 'return', {})
 
-        self.wait_for_open()
-        self.wait_for_close()
-
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', True)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
@@ -390,7 +397,6 @@ class TestChangeReadOnly(ChangeBaseClass):
         self.vm.launch()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', True)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
@@ -400,11 +406,7 @@ class TestChangeReadOnly(ChangeBaseClass):
                                                        read_only_mode='retain')
         self.assert_qmp(result, 'return', {})
 
-        self.wait_for_open()
-        self.wait_for_close()
-
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', True)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
@@ -414,7 +416,6 @@ class TestChangeReadOnly(ChangeBaseClass):
         self.vm.launch()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
@@ -427,7 +428,6 @@ class TestChangeReadOnly(ChangeBaseClass):
         self.assertEquals(self.vm.get_qmp_events(wait=False), [])
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
@@ -437,7 +437,6 @@ class TestChangeReadOnly(ChangeBaseClass):
         self.vm.launch()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', True)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
@@ -448,11 +447,7 @@ class TestChangeReadOnly(ChangeBaseClass):
                              read_only_mode='read-write')
         self.assert_qmp(result, 'return', {})
 
-        self.wait_for_open()
-        self.wait_for_close()
-
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
@@ -462,7 +457,6 @@ class TestChangeReadOnly(ChangeBaseClass):
         self.vm.launch()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
@@ -473,11 +467,7 @@ class TestChangeReadOnly(ChangeBaseClass):
                              read_only_mode='read-only')
         self.assert_qmp(result, 'return', {})
 
-        self.wait_for_open()
-        self.wait_for_close()
-
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', True)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
@@ -486,7 +476,6 @@ class TestChangeReadOnly(ChangeBaseClass):
         self.vm.launch()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
@@ -497,11 +486,7 @@ class TestChangeReadOnly(ChangeBaseClass):
                              read_only_mode='read-only')
         self.assert_qmp(result, 'return', {})
 
-        self.wait_for_open()
-        self.wait_for_close()
-
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', True)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
@@ -511,7 +496,6 @@ class TestChangeReadOnly(ChangeBaseClass):
         self.vm.launch()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
@@ -522,10 +506,7 @@ class TestChangeReadOnly(ChangeBaseClass):
                              read_only_mode='read-write')
         self.assert_qmp(result, 'error/class', 'GenericError')
 
-        self.assertEquals(self.vm.get_qmp_events(wait=False), [])
-
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
@@ -535,7 +516,6 @@ class TestChangeReadOnly(ChangeBaseClass):
         self.vm.launch()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', True)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
@@ -545,11 +525,7 @@ class TestChangeReadOnly(ChangeBaseClass):
                                                        read_only_mode='retain')
         self.assert_qmp(result, 'return', {})
 
-        self.wait_for_open()
-        self.wait_for_close()
-
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', True)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
@@ -559,7 +535,6 @@ class TestChangeReadOnly(ChangeBaseClass):
         self.vm.launch()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
@@ -569,10 +544,7 @@ class TestChangeReadOnly(ChangeBaseClass):
                                                        read_only_mode='retain')
         self.assert_qmp(result, 'error/class', 'GenericError')
 
-        self.assertEquals(self.vm.get_qmp_events(wait=False), [])
-
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
@@ -582,7 +554,6 @@ class TestChangeReadOnly(ChangeBaseClass):
         self.vm.launch()
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
@@ -594,13 +565,7 @@ class TestChangeReadOnly(ChangeBaseClass):
                                                'driver': 'file'}})
         self.assert_qmp(result, 'return', {})
 
-        result = self.vm.qmp('blockdev-open-tray', device='drive0', force=True)
-        self.assert_qmp(result, 'return', {})
-
-        self.wait_for_open()
-
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', True)
         self.assert_qmp(result, 'return[0]/inserted/ro', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
@@ -608,7 +573,6 @@ class TestChangeReadOnly(ChangeBaseClass):
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', True)
         self.assert_qmp_absent(result, 'return[0]/inserted')
 
         result = self.vm.qmp('x-blockdev-insert-medium', device='drive0',
@@ -616,17 +580,10 @@ class TestChangeReadOnly(ChangeBaseClass):
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', True)
         self.assert_qmp(result, 'return[0]/inserted/ro', True)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
-        result = self.vm.qmp('blockdev-close-tray', device='drive0')
-        self.assert_qmp(result, 'return', {})
-
-        self.wait_for_close()
-
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/ro', True)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
@@ -648,7 +605,6 @@ class TestBlockJobsAfterCycle(ChangeBaseClass):
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/image/format', 'null-co')
 
         # For device-less BBs, calling blockdev-open-tray or blockdev-close-tray
@@ -671,7 +627,6 @@ class TestBlockJobsAfterCycle(ChangeBaseClass):
         self.assert_qmp(result, 'return', {})
 
         result = self.vm.qmp('query-block')
-        self.assert_qmp(result, 'return[0]/tray_open', False)
         self.assert_qmp(result, 'return[0]/inserted/image/filename', old_img)
 
     def tearDown(self):
-- 
2.7.0

[Qemu-devel] [PATCH 4/4] block/qapi: Emit tray_open only if there is a tray

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qapi.c               | 2 +-
 tests/qemu-iotests/067.out | 4 ----
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 58d3975..12a0f25 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -299,7 +299,7 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
     info->locked = blk_dev_is_medium_locked(blk);
     info->removable = blk_dev_has_removable_media(blk);
 
-    if (blk_dev_has_removable_media(blk)) {
+    if (blk_dev_has_tray(blk)) {
         info->has_tray_open = true;
         info->tray_open = blk_dev_is_tray_open(blk);
     }
diff --git a/tests/qemu-iotests/067.out b/tests/qemu-iotests/067.out
index 27ad56f..ae3fccb 100644
--- a/tests/qemu-iotests/067.out
+++ b/tests/qemu-iotests/067.out
@@ -169,7 +169,6 @@ Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,if=none,id=disk
                 "file": "TEST_DIR/t.qcow2",
                 "encryption_key_missing": false
             },
-            "tray_open": false,
             "type": "unknown"
         }
     ]
@@ -289,7 +288,6 @@ Testing:
                 "file": "TEST_DIR/t.qcow2",
                 "encryption_key_missing": false
             },
-            "tray_open": false,
             "type": "unknown"
         }
     ]
@@ -410,7 +408,6 @@ Testing:
                 "file": "TEST_DIR/t.qcow2",
                 "encryption_key_missing": false
             },
-            "tray_open": false,
             "type": "unknown"
         }
     ]
@@ -501,7 +498,6 @@ Testing:
                 "file": "TEST_DIR/t.qcow2",
                 "encryption_key_missing": false
             },
-            "tray_open": false,
             "type": "unknown"
         }
     ]
-- 
2.7.0

Re: [Qemu-devel] [PATCH 0/4] blockdev: Fix 'change' for slot devices

[Peter Maydell]

On 12 January 2016 at 15:47, Max Reitz <mreitz@redhat.com> wrote:
> The series "BlockBackend and media" intended all block devices with
> removable media to implement a tray model; if the devices does not have
> a tray, it should emulate one.

I tried this for the zaurus (spitz) board, and although "change sd0 file"
now doesn't give an error, it doesn't cause the guest to notice that
an SD card has been inserted either.

I start the guest with no SD card plugged in, and then from the
monitor issue "change sd0 file.img". The sd_cardchange callback
is called (from blk_dev_change_media_cb, with load==true), but
when it sd_cardchange() calls blk_is_inserted(sd->blk) this returns false
and so the SD card model reports to the controller that no card is present.

If I issue the command again, the callback is called twice,
firstly with load=false -- this time blk_is_inserted() returns
true and so the sd card says "card has been inserted" (and the
guest notices this and prints its usual found-mmc-card logging). Then
on the second callback with load==true blk_is_inserted() returns
false again and so the card says "card not inserted".

So it looks like the sense of blk_is_inserted() when the
loading/unloading callbacks are made is the opposite of what the
sd.c code is expecting.

(You can see this behaviour with just the first 2 cc-stable patches.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/4] blockdev: Fix 'change' for slot devices

[Peter Maydell]

On 15 January 2016 at 16:10, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 12 January 2016 at 15:47, Max Reitz <mreitz@redhat.com> wrote:
>> The series "BlockBackend and media" intended all block devices with
>> removable media to implement a tray model; if the devices does not have
>> a tray, it should emulate one.
>
> I tried this for the zaurus (spitz) board, and although "change sd0 file"
> now doesn't give an error, it doesn't cause the guest to notice that
> an SD card has been inserted either.

PS: my test image for zaurus is here:
 http://people.linaro.org/~peter.maydell/zaurus2.tgz

You can run it with
  path/to/zaurus2/runme path/to/qemu-system-arm
which by default will boot up with no SD card inserted, serial console
output to stdout. Using the 'change sd0 somefile' ought to then
make it notice an SD card and print the usual kernel messages.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/4] block: Add blk_dev_has_tray()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 681 bytes --]

On 01/12/2016 08:47 AM, Max Reitz wrote:
> Pull out the check whether a block device has a tray from
> blk_dev_is_tray_open() into an own function so both attributes (whether

s.an own/its own/

> there is a tray vs. whether that tray is open) can be queried
> independently.
> 
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/block-backend.c     | 10 +++++++++-
>  include/block/block_int.h |  1 +
>  2 files changed, 10 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/4] blockdev: Fix 'change' for slot devices

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1071 bytes --]

On 01/12/2016 08:47 AM, Max Reitz wrote:
> 'change' and related operations did not work when used on guest devices
> featuring removable media but no actual tray, because
> blk_dev_is_tray_open() always returned false for them and the
> blockdev-{insert,remove}-medium commands required it to return true.
> 
> Fix this by making blockdev-{insert,remove}-medium work on tray-less
> devices. Also, blockdev-{open,close}-tray are now explicitly no-ops when
> invoked on such devices, and blk_dev_change_media_cb() is instead
> called by blockdev-{insert,remove}-medium (for tray-less devices only).
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c           | 27 +++++++++++++++++++++++++--
>  qapi/block-core.json |  3 +--
>  2 files changed, 26 insertions(+), 4 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/4] Revert "hw/block/fdc: Implement tray status"

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

On 01/12/2016 08:47 AM, Max Reitz wrote:
> This reverts the changes commit
> 2e1280e8ff95b3145bc6262accc9d447718e5318 applied to hw/block/fdc.c.
> 

Might read better as s/changes commit/changes that commit/

> That commit changed tests/fdc-test.c, too, because after it, one less
> TRAY_MOVED event would be emitted when executing 'change' on an empty
> drive. However, now, no TRAY_MOVED events will be emitted at all, and
> the tray_open status returned by query-block will always be false,
> necessitating (different) changes to tests/fdc-test.c and iotest 118,
> which is why this patch is not a pure revert of said commit.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  hw/block/fdc.c         |  20 ++-------
>  tests/fdc-test.c       |   2 -
>  tests/qemu-iotests/118 | 117 +++++++++++++++----------------------------------
>  3 files changed, 40 insertions(+), 99 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 4/4] block/qapi: Emit tray_open only if there is a tray

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 389 bytes --]

On 01/12/2016 08:47 AM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qapi.c               | 2 +-
>  tests/qemu-iotests/067.out | 4 ----
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 1/4] block: Add blk_dev_has_tray()

[Alberto Garcia]

On Tue 12 Jan 2016 04:47:51 PM CET, Max Reitz <mreitz@redhat.com> wrote:
> Pull out the check whether a block device has a tray from
> blk_dev_is_tray_open() into an own function so both attributes (whether
> there is a tray vs. whether that tray is open) can be queried
> independently.
>
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH 2/4] blockdev: Fix 'change' for slot devices

[Alberto Garcia]

On Tue 12 Jan 2016 04:47:52 PM CET, Max Reitz wrote:
> 'change' and related operations did not work when used on guest devices
> featuring removable media but no actual tray, because
> blk_dev_is_tray_open() always returned false for them and the
> blockdev-{insert,remove}-medium commands required it to return true.
>
> Fix this by making blockdev-{insert,remove}-medium work on tray-less
> devices. Also, blockdev-{open,close}-tray are now explicitly no-ops when
> invoked on such devices, and blk_dev_change_media_cb() is instead
> called by blockdev-{insert,remove}-medium (for tray-less devices only).
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] block/qapi: Emit tray_open only if there is a tray

[Alberto Garcia]

On Tue 12 Jan 2016 04:47:54 PM CET, Max Reitz wrote:

> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qapi.c               | 2 +-
>  tests/qemu-iotests/067.out | 4 ----
>  2 files changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index 58d3975..12a0f25 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -299,7 +299,7 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
>      info->locked = blk_dev_is_medium_locked(blk);
>      info->removable = blk_dev_has_removable_media(blk);
>  
> -    if (blk_dev_has_removable_media(blk)) {
> +    if (blk_dev_has_tray(blk)) {
>          info->has_tray_open = true;
>          info->tray_open = blk_dev_is_tray_open(blk);
>      }

It probably makes sense to update the documentation as well:

# @tray_open: #optional True if the device has a tray and it is open
#             (only present if removable is true)

Berto