All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/3] block: Extend QMP events for anonymous BlockBackends
@ 2016-10-05  9:26 Kevin Wolf
  2016-10-05  9:26 ` [Qemu-devel] [PATCH 1/3] block: Add node name to BLOCK_IO_ERROR event Kevin Wolf
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Kevin Wolf @ 2016-10-05  9:26 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, qemu-devel

We have two QMP events that include only a BlockBackend name and that are
therefore useless with anonymous BBs. This series adds a node-name/qdev ID to
them.

Kevin Wolf (3):
  block: Add node name to BLOCK_IO_ERROR event
  block-backend: Remember if attached device is non-qdev
  block: Add qdev ID to DEVICE_TRAY_MOVED

 block.c                        |  7 -----
 block/block-backend.c          | 64 +++++++++++++++++++++++++++++++++++-------
 docs/qmp-commands.txt          |  3 ++
 docs/qmp-events.txt            | 12 ++++++--
 hw/block/xen_disk.c            |  2 +-
 include/sysemu/block-backend.h |  4 +--
 qapi/block-core.json           |  8 ++++--
 qapi/block.json                |  8 ++++--
 8 files changed, 82 insertions(+), 26 deletions(-)

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 1/3] block: Add node name to BLOCK_IO_ERROR event
  2016-10-05  9:26 [Qemu-devel] [PATCH 0/3] block: Extend QMP events for anonymous BlockBackends Kevin Wolf
@ 2016-10-05  9:26 ` Kevin Wolf
  2016-10-05 17:48   ` Max Reitz
  2016-10-05  9:26 ` [Qemu-devel] [PATCH 2/3] block-backend: Remember if attached device is non-qdev Kevin Wolf
  2016-10-05  9:26 ` [Qemu-devel] [PATCH 3/3] block: Add qdev ID to DEVICE_TRAY_MOVED Kevin Wolf
  2 siblings, 1 reply; 9+ messages in thread
From: Kevin Wolf @ 2016-10-05  9:26 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, qemu-devel

The event currently only contains the BlockBackend name. However, with
anonymous BlockBackends, this is always the empty string. Add the node
name so that the user can still see which block device caused the event.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c | 5 +++--
 docs/qmp-events.txt   | 6 +++++-
 qapi/block-core.json  | 8 ++++++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 639294b..911254a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1206,8 +1206,9 @@ static void send_qmp_error_event(BlockBackend *blk,
     IoOperationType optype;
 
     optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
-    qapi_event_send_block_io_error(blk_name(blk), optype, action,
-                                   blk_iostatus_is_enabled(blk),
+    qapi_event_send_block_io_error(blk_name(blk),
+                                   bdrv_get_node_name(blk_bs(blk)), optype,
+                                   action, blk_iostatus_is_enabled(blk),
                                    error == ENOSPC, strerror(error),
                                    &error_abort);
 }
diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index 7967ec4..25307ab 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -65,7 +65,10 @@ Emitted when a disk I/O error occurs.
 
 Data:
 
-- "device": device name (json-string)
+- "device": device name. This is always present for compatibility
+            reasons, but it can be empty ("") if the image does not
+            have a device name associated. (json-string)
+- "node-name": node name (json-string)
 - "operation": I/O operation (json-string, "read" or "write")
 - "action": action that has been taken, it's one of the following (json-string):
     "ignore": error has been ignored
@@ -76,6 +79,7 @@ Example:
 
 { "event": "BLOCK_IO_ERROR",
     "data": { "device": "ide0-hd1",
+              "node-name": "#block212",
               "operation": "write",
               "action": "stop" },
     "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9d797b8..c4538d6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2545,7 +2545,11 @@
 #
 # Emitted when a disk I/O error occurs
 #
-# @device: device name
+# @device: device name. This is always present for compatibility
+#          reasons, but it can be empty ("") if the image does not
+#          have a device name associated.
+#
+# @node-name: node name (Since: 2.8)
 #
 # @operation: I/O operation
 #
@@ -2566,7 +2570,7 @@
 # Since: 0.13.0
 ##
 { 'event': 'BLOCK_IO_ERROR',
-  'data': { 'device': 'str', 'operation': 'IoOperationType',
+  'data': { 'device': 'str', 'node-name': 'str', 'operation': 'IoOperationType',
             'action': 'BlockErrorAction', '*nospace': 'bool',
             'reason': 'str' } }
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 2/3] block-backend: Remember if attached device is non-qdev
  2016-10-05  9:26 [Qemu-devel] [PATCH 0/3] block: Extend QMP events for anonymous BlockBackends Kevin Wolf
  2016-10-05  9:26 ` [Qemu-devel] [PATCH 1/3] block: Add node name to BLOCK_IO_ERROR event Kevin Wolf
@ 2016-10-05  9:26 ` Kevin Wolf
  2016-10-05 18:01   ` Max Reitz
  2016-10-05  9:26 ` [Qemu-devel] [PATCH 3/3] block: Add qdev ID to DEVICE_TRAY_MOVED Kevin Wolf
  2 siblings, 1 reply; 9+ messages in thread
From: Kevin Wolf @ 2016-10-05  9:26 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, qemu-devel

Almost all block devices are qdevified by now. This allows us to go back
from the BlockBackend to the DeviceState. xen_disk is the last device
that is missing. We'll remember in the BlockBackend if a xen_disk is
attached and can then disable any features that require going from a BB
to the DeviceState.

While at it, clearly mark the function used by xen_disk as legacy even
in its name, not just in TODO comments.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c          | 26 +++++++++++++++++++-------
 hw/block/xen_disk.c            |  2 +-
 include/sysemu/block-backend.h |  4 ++--
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 911254a..39c5613 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -38,6 +38,7 @@ struct BlockBackend {
     BlockBackendPublic public;
 
     void *dev;                  /* attached device model, if any */
+    bool legacy_dev;            /* true if dev is not a DeviceState */
     /* TODO change to DeviceState when all users are qdevified */
     const BlockDevOps *dev_ops;
     void *dev_opaque;
@@ -507,32 +508,38 @@ void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
     }
 }
 
-/*
- * Attach device model @dev to @blk.
- * Return 0 on success, -EBUSY when a device model is attached already.
- */
-int blk_attach_dev(BlockBackend *blk, void *dev)
-/* TODO change to DeviceState *dev when all users are qdevified */
+static int blk_do_attach_dev(BlockBackend *blk, void *dev)
 {
     if (blk->dev) {
         return -EBUSY;
     }
     blk_ref(blk);
     blk->dev = dev;
+    blk->legacy_dev = false;
     blk_iostatus_reset(blk);
     return 0;
 }
 
 /*
  * Attach device model @dev to @blk.
+ * Return 0 on success, -EBUSY when a device model is attached already.
+ */
+int blk_attach_dev(BlockBackend *blk, DeviceState *dev)
+{
+    return blk_do_attach_dev(blk, dev);
+}
+
+/*
+ * Attach device model @dev to @blk.
  * @blk must not have a device model attached already.
  * TODO qdevified devices don't use this, remove when devices are qdevified
  */
-void blk_attach_dev_nofail(BlockBackend *blk, void *dev)
+void blk_attach_dev_legacy(BlockBackend *blk, void *dev)
 {
     if (blk_attach_dev(blk, dev) < 0) {
         abort();
     }
+    blk->legacy_dev = true;
 }
 
 /*
@@ -586,6 +593,11 @@ BlockBackend *blk_by_dev(void *dev)
 void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops,
                      void *opaque)
 {
+    /* All drivers that use blk_set_dev_ops() are qdevified and we want to keep
+     * it that way, so we can assume blk->dev is a DeviceState if blk->dev_ops
+     * is set. */
+    assert(!blk->legacy_dev);
+
     blk->dev_ops = ops;
     blk->dev_opaque = opaque;
 }
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 5aa350a..1292a4b 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -1079,7 +1079,7 @@ static int blk_connect(struct XenDevice *xendev)
          * so we can blk_unref() unconditionally */
         blk_ref(blkdev->blk);
     }
-    blk_attach_dev_nofail(blkdev->blk, blkdev);
+    blk_attach_dev_legacy(blkdev->blk, blkdev);
     blkdev->file_size = blk_getlength(blkdev->blk);
     if (blkdev->file_size < 0) {
         BlockDriverState *bs = blk_bs(blkdev->blk);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index a7993af..b07159b 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -107,8 +107,8 @@ BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk);
 void blk_iostatus_disable(BlockBackend *blk);
 void blk_iostatus_reset(BlockBackend *blk);
 void blk_iostatus_set_err(BlockBackend *blk, int error);
-int blk_attach_dev(BlockBackend *blk, void *dev);
-void blk_attach_dev_nofail(BlockBackend *blk, void *dev);
+int blk_attach_dev(BlockBackend *blk, DeviceState *dev);
+void blk_attach_dev_legacy(BlockBackend *blk, void *dev);
 void blk_detach_dev(BlockBackend *blk, void *dev);
 void *blk_get_attached_dev(BlockBackend *blk);
 BlockBackend *blk_by_dev(void *dev);
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 3/3] block: Add qdev ID to DEVICE_TRAY_MOVED
  2016-10-05  9:26 [Qemu-devel] [PATCH 0/3] block: Extend QMP events for anonymous BlockBackends Kevin Wolf
  2016-10-05  9:26 ` [Qemu-devel] [PATCH 1/3] block: Add node name to BLOCK_IO_ERROR event Kevin Wolf
  2016-10-05  9:26 ` [Qemu-devel] [PATCH 2/3] block-backend: Remember if attached device is non-qdev Kevin Wolf
@ 2016-10-05  9:26 ` Kevin Wolf
  2016-10-05 18:21   ` Max Reitz
  2 siblings, 1 reply; 9+ messages in thread
From: Kevin Wolf @ 2016-10-05  9:26 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, mreitz, eblake, qemu-devel

The event currently only contains the BlockBackend name. However, with
anonymous BlockBackends, this is always the empty string. Add the qdev
ID (or if none was given, the QOM path) so that the user can still see
which device caused the event.

Event generation has to be moved from bdrv_eject() to the BlockBackend
because the BDS doesn't know the attached device, but that's easy
because blk_eject() is the only user of it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c               |  7 -------
 block/block-backend.c | 33 ++++++++++++++++++++++++++++++++-
 docs/qmp-commands.txt |  3 +++
 docs/qmp-events.txt   |  6 +++++-
 qapi/block.json       |  8 ++++++--
 5 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index bb1f1ec..b0bdad6 100644
--- a/block.c
+++ b/block.c
@@ -3360,17 +3360,10 @@ int bdrv_media_changed(BlockDriverState *bs)
 void bdrv_eject(BlockDriverState *bs, bool eject_flag)
 {
     BlockDriver *drv = bs->drv;
-    const char *device_name;
 
     if (drv && drv->bdrv_eject) {
         drv->bdrv_eject(bs, eject_flag);
     }
-
-    device_name = bdrv_get_device_name(bs);
-    if (device_name[0] != '\0') {
-        qapi_event_send_device_tray_moved(device_name,
-                                          eject_flag, &error_abort);
-    }
 }
 
 /**
diff --git a/block/block-backend.c b/block/block-backend.c
index 39c5613..6859791 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -566,6 +566,23 @@ void *blk_get_attached_dev(BlockBackend *blk)
     return blk->dev;
 }
 
+/* Return the qdev ID, or if no ID is assigned the QOM path, of the block
+ * device attached to the BlockBackend. */
+static char *blk_get_attached_dev_id(BlockBackend *blk)
+{
+    DeviceState *dev;
+
+    assert(!blk->legacy_dev);
+    dev = blk->dev;
+
+    if (!dev) {
+        return g_strdup("");
+    } else if (dev->id) {
+        return g_strdup(dev->id);
+    }
+    return object_get_canonical_path(OBJECT(dev));
+}
+
 /*
  * Return the BlockBackend which has the device model @dev attached if it
  * exists, else null.
@@ -613,13 +630,17 @@ void blk_dev_change_media_cb(BlockBackend *blk, bool load)
     if (blk->dev_ops && blk->dev_ops->change_media_cb) {
         bool tray_was_open, tray_is_open;
 
+        assert(!blk->legacy_dev);
+
         tray_was_open = blk_dev_is_tray_open(blk);
         blk->dev_ops->change_media_cb(blk->dev_opaque, load);
         tray_is_open = blk_dev_is_tray_open(blk);
 
         if (tray_was_open != tray_is_open) {
-            qapi_event_send_device_tray_moved(blk_name(blk), tray_is_open,
+            char *id = blk_get_attached_dev_id(blk);
+            qapi_event_send_device_tray_moved(blk_name(blk), id, tray_is_open,
                                               &error_abort);
+            g_free(id);
         }
     }
 }
@@ -1325,9 +1346,19 @@ void blk_lock_medium(BlockBackend *blk, bool locked)
 void blk_eject(BlockBackend *blk, bool eject_flag)
 {
     BlockDriverState *bs = blk_bs(blk);
+    char *id;
+
+    /* blk_eject is only called by qdevified devices */
+    assert(!blk->legacy_dev);
 
     if (bs) {
         bdrv_eject(bs, eject_flag);
+
+        id = blk_get_attached_dev_id(blk);
+        qapi_event_send_device_tray_moved(blk_name(blk), id,
+                                          eject_flag, &error_abort);
+        g_free(id);
+
     }
 }
 
diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
index e0adceb..e044029 100644
--- a/docs/qmp-commands.txt
+++ b/docs/qmp-commands.txt
@@ -3239,6 +3239,7 @@ Example:
                     "microseconds": 716996 },
      "event": "DEVICE_TRAY_MOVED",
      "data": { "device": "ide1-cd0",
+               "id": "ide0-1-0",
                "tray-open": true } }
 
 <- { "return": {} }
@@ -3267,6 +3268,7 @@ Example:
                     "microseconds": 272147 },
      "event": "DEVICE_TRAY_MOVED",
      "data": { "device": "ide1-cd0",
+               "id": "ide0-1-0",
                "tray-open": false } }
 
 <- { "return": {} }
@@ -3303,6 +3305,7 @@ Example:
                     "microseconds": 549958 },
      "event": "DEVICE_TRAY_MOVED",
      "data": { "device": "ide1-cd0",
+               "id": "ide0-1-0",
                "tray-open": true } }
 
 <- { "return": {} }
diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index 25307ab..4d56022 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -218,12 +218,16 @@ or by HMP/QMP commands.
 
 Data:
 
-- "device": device name (json-string)
+- "device": Block device name. This is always present for compatibility
+            reasons, but it can be empty ("") if the image does not have a
+            device name associated. (json-string)
+- "id": The name or QOM path of the guest device (json-string)
 - "tray-open": true if the tray has been opened or false if it has been closed
                (json-bool)
 
 { "event": "DEVICE_TRAY_MOVED",
   "data": { "device": "ide1-cd0",
+            "id": "/machine/unattached/device[22]",
             "tray-open": true
   },
   "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
diff --git a/qapi/block.json b/qapi/block.json
index c896bd1..4661fc9 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -195,14 +195,18 @@
 # Emitted whenever the tray of a removable device is moved by the guest or by
 # HMP/QMP commands
 #
-# @device: device name
+# @device: Block device name. This is always present for compatibility
+#          reasons, but it can be empty ("") if the image does not
+#          have a device name associated.
+#
+# @id: The name or QOM path of the guest device
 #
 # @tray-open: true if the tray has been opened or false if it has been closed
 #
 # Since: 1.1
 ##
 { 'event': 'DEVICE_TRAY_MOVED',
-  'data': { 'device': 'str', 'tray-open': 'bool' } }
+  'data': { 'device': 'str', 'id': 'str', 'tray-open': 'bool' } }
 
 ##
 # @QuorumOpType
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] block: Add node name to BLOCK_IO_ERROR event
  2016-10-05  9:26 ` [Qemu-devel] [PATCH 1/3] block: Add node name to BLOCK_IO_ERROR event Kevin Wolf
@ 2016-10-05 17:48   ` Max Reitz
  0 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2016-10-05 17:48 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: eblake, qemu-devel

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

On 05.10.2016 11:26, Kevin Wolf wrote:
> The event currently only contains the BlockBackend name. However, with
> anonymous BlockBackends, this is always the empty string. Add the node
> name so that the user can still see which block device caused the event.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c | 5 +++--
>  docs/qmp-events.txt   | 6 +++++-
>  qapi/block-core.json  | 8 ++++++--
>  3 files changed, 14 insertions(+), 5 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Although it is a bit weird to report the root node's name, because I'd
expect the I/O error to occur on the protocol layer and thus the event
to report the protocol node's name.

OTOH, I think it'd be rather difficult to either keep the information
from which node the error originated or to trace it back when we want to
report it (and the decision to report it is generally made on the root
node, so...).

Therefore, reporting it on the root node is probably the best we can do.

Would it make sense to add a short note to the documentation
(qmp-events.txt and block-core.json) that the node name reported is not
necessarily the node the I/O error originated from but may (and
generally is) instead be some node above it?

Max


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 2/3] block-backend: Remember if attached device is non-qdev
  2016-10-05  9:26 ` [Qemu-devel] [PATCH 2/3] block-backend: Remember if attached device is non-qdev Kevin Wolf
@ 2016-10-05 18:01   ` Max Reitz
  2016-10-05 19:11     ` Eric Blake
  2016-10-06 11:29     ` Kevin Wolf
  0 siblings, 2 replies; 9+ messages in thread
From: Max Reitz @ 2016-10-05 18:01 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: eblake, qemu-devel

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

On 05.10.2016 11:26, Kevin Wolf wrote:
> Almost all block devices are qdevified by now. This allows us to go back
> from the BlockBackend to the DeviceState. xen_disk is the last device
> that is missing. We'll remember in the BlockBackend if a xen_disk is
> attached and can then disable any features that require going from a BB
> to the DeviceState.
> 
> While at it, clearly mark the function used by xen_disk as legacy even
> in its name, not just in TODO comments.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c          | 26 +++++++++++++++++++-------
>  hw/block/xen_disk.c            |  2 +-
>  include/sysemu/block-backend.h |  4 ++--
>  3 files changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 911254a..39c5613 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c

[...]

> @@ -507,32 +508,38 @@ void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
>      }
>  }
>  
> -/*
> - * Attach device model @dev to @blk.
> - * Return 0 on success, -EBUSY when a device model is attached already.
> - */
> -int blk_attach_dev(BlockBackend *blk, void *dev)
> -/* TODO change to DeviceState *dev when all users are qdevified */
> +static int blk_do_attach_dev(BlockBackend *blk, void *dev)
>  {
>      if (blk->dev) {
>          return -EBUSY;
>      }
>      blk_ref(blk);
>      blk->dev = dev;
> +    blk->legacy_dev = false;
>      blk_iostatus_reset(blk);
>      return 0;
>  }
>  
>  /*
>   * Attach device model @dev to @blk.
> + * Return 0 on success, -EBUSY when a device model is attached already.
> + */
> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev)
> +{
> +    return blk_do_attach_dev(blk, dev);
> +}
> +
> +/*
> + * Attach device model @dev to @blk.
>   * @blk must not have a device model attached already.
>   * TODO qdevified devices don't use this, remove when devices are qdevified
>   */
> -void blk_attach_dev_nofail(BlockBackend *blk, void *dev)
> +void blk_attach_dev_legacy(BlockBackend *blk, void *dev)
>  {
>      if (blk_attach_dev(blk, dev) < 0) {

I'd make this a blk_do_attach_dev(), but it's not syntactically wrong,
so the choice is up to you.

Thus, either way:

Reviewed-by: Max Reitz <mreitz@redhat.com>

>          abort();
>      }
> +    blk->legacy_dev = true;
>  }
>  
>  /*


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] block: Add qdev ID to DEVICE_TRAY_MOVED
  2016-10-05  9:26 ` [Qemu-devel] [PATCH 3/3] block: Add qdev ID to DEVICE_TRAY_MOVED Kevin Wolf
@ 2016-10-05 18:21   ` Max Reitz
  0 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2016-10-05 18:21 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: eblake, qemu-devel

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

On 05.10.2016 11:26, Kevin Wolf wrote:
> The event currently only contains the BlockBackend name. However, with
> anonymous BlockBackends, this is always the empty string. Add the qdev
> ID (or if none was given, the QOM path) so that the user can still see
> which device caused the event.
> 
> Event generation has to be moved from bdrv_eject() to the BlockBackend
> because the BDS doesn't know the attached device, but that's easy
> because blk_eject() is the only user of it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c               |  7 -------
>  block/block-backend.c | 33 ++++++++++++++++++++++++++++++++-
>  docs/qmp-commands.txt |  3 +++
>  docs/qmp-events.txt   |  6 +++++-
>  qapi/block.json       |  8 ++++++--
>  5 files changed, 46 insertions(+), 11 deletions(-)

I didn't even know we use the same event for both virtual and physical
trays. Fun, fun.

Even more fun in that blk_eject() always issues the event, regardless of
whether there actually is a physical tray. Therefore, I assume it's
effectively just an event for the virtual tray which has to hook up in
blk_eject() because a guest eject won't go through
blk_dev_change_media_cb(), but it will go through blk_eject() (which QMP
ejects do not). Totally makes sense.

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 2/3] block-backend: Remember if attached device is non-qdev
  2016-10-05 18:01   ` Max Reitz
@ 2016-10-05 19:11     ` Eric Blake
  2016-10-06 11:29     ` Kevin Wolf
  1 sibling, 0 replies; 9+ messages in thread
From: Eric Blake @ 2016-10-05 19:11 UTC (permalink / raw)
  To: Max Reitz, Kevin Wolf, qemu-block; +Cc: qemu-devel

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

On 10/05/2016 01:01 PM, Max Reitz wrote:

>> +static int blk_do_attach_dev(BlockBackend *blk, void *dev)

>> + */
>> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev)
>> +{
>> +    return blk_do_attach_dev(blk, dev);

>> +void blk_attach_dev_legacy(BlockBackend *blk, void *dev)
>>  {
>>      if (blk_attach_dev(blk, dev) < 0) {
> 
> I'd make this a blk_do_attach_dev(), but it's not syntactically wrong,
> so the choice is up to you.

It's technically undefined C behavior to cast from void* to an unrelated
pointer and back to void* (you are only guaranteed a round trip from
type to void* and back to original type, not with unrelated types).  So
syntactically valid but semantically undefined.

On the other hand, ABI-wise, I'd be shocked if our compilers are able to
exploit our use of undefined behavior.

At any rate, using blk_do_attach_dev() would avoid the definedness
problem, so I'd go ahead and make the change.

-- 
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 --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 2/3] block-backend: Remember if attached device is non-qdev
  2016-10-05 18:01   ` Max Reitz
  2016-10-05 19:11     ` Eric Blake
@ 2016-10-06 11:29     ` Kevin Wolf
  1 sibling, 0 replies; 9+ messages in thread
From: Kevin Wolf @ 2016-10-06 11:29 UTC (permalink / raw)
  To: Max Reitz; +Cc: qemu-block, eblake, qemu-devel

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

Am 05.10.2016 um 20:01 hat Max Reitz geschrieben:
> On 05.10.2016 11:26, Kevin Wolf wrote:
> > Almost all block devices are qdevified by now. This allows us to go back
> > from the BlockBackend to the DeviceState. xen_disk is the last device
> > that is missing. We'll remember in the BlockBackend if a xen_disk is
> > attached and can then disable any features that require going from a BB
> > to the DeviceState.
> > 
> > While at it, clearly mark the function used by xen_disk as legacy even
> > in its name, not just in TODO comments.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>

> > +/*
> > + * Attach device model @dev to @blk.
> >   * @blk must not have a device model attached already.
> >   * TODO qdevified devices don't use this, remove when devices are qdevified
> >   */
> > -void blk_attach_dev_nofail(BlockBackend *blk, void *dev)
> > +void blk_attach_dev_legacy(BlockBackend *blk, void *dev)
> >  {
> >      if (blk_attach_dev(blk, dev) < 0) {
> 
> I'd make this a blk_do_attach_dev(), but it's not syntactically wrong,
> so the choice is up to you.

Thanks for catching this, what you suggest is what I intended. Maybe we
should just enable the gcc --do-what-i-mean option.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-10-06 11:29 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-05  9:26 [Qemu-devel] [PATCH 0/3] block: Extend QMP events for anonymous BlockBackends Kevin Wolf
2016-10-05  9:26 ` [Qemu-devel] [PATCH 1/3] block: Add node name to BLOCK_IO_ERROR event Kevin Wolf
2016-10-05 17:48   ` Max Reitz
2016-10-05  9:26 ` [Qemu-devel] [PATCH 2/3] block-backend: Remember if attached device is non-qdev Kevin Wolf
2016-10-05 18:01   ` Max Reitz
2016-10-05 19:11     ` Eric Blake
2016-10-06 11:29     ` Kevin Wolf
2016-10-05  9:26 ` [Qemu-devel] [PATCH 3/3] block: Add qdev ID to DEVICE_TRAY_MOVED Kevin Wolf
2016-10-05 18:21   ` Max Reitz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.