[Qemu-devel] [PATCH 0/3] Add bdrv_get_device_or_node_name()

[Qemu-devel] [PATCH 0/3] Add bdrv_get_device_or_node_name()

[Alberto Garcia]

We have several cases of error messages related to BlockDriverState that expect it to have a device name.

However many of those can happen in nodes that don't have a device name associated so the generated error messages are not very meaningful.

This series adds a new function bdrv_get_device_or_node_name(), that returns the node name if there is no device name for that node. Since both the device and the node name live in the same namespace, there's no ambiguity. And since it gives preference to the device name, it maintains backward compatibility.

It also updates all calls to bdrv_get_device_name() where it makes sense: several error messages and the BLOCK_IMAGE_CORRUPTED event.

I wrote this thinking on the case where any arbitrary node will be able to own a block job, but I think it's useful already for the reasons explained above.

Berto

Alberto Garcia (3):
  block: add bdrv_get_device_or_node_name()
  block: use bdrv_get_device_or_node_name() in error messages
  block: allow BLOCK_IMAGE_CORRUPTED to have a node name

 block.c                 | 18 ++++++++++++------
 block/qcow.c            |  4 ++--
 block/qcow2.c           |  7 ++++---
 block/qed.c             |  2 +-
 block/quorum.c          |  5 +----
 block/vdi.c             |  2 +-
 block/vhdx.c            |  2 +-
 block/vmdk.c            |  4 ++--
 block/vpc.c             |  2 +-
 block/vvfat.c           |  3 ++-
 docs/qmp/qmp-events.txt |  2 +-
 include/block/block.h   |  1 +
 qapi/block-core.json    |  2 +-
 13 files changed, 30 insertions(+), 24 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/3] block: add bdrv_get_device_or_node_name()

[Alberto Garcia]

This function gets the device name associated with a BlockDriverState,
or its node name if the device name is empty.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c               | 5 +++++
 block/quorum.c        | 5 +----
 include/block/block.h | 1 +
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 0fe97de..af284e3 100644
--- a/block.c
+++ b/block.c
@@ -3920,6 +3920,11 @@ const char *bdrv_get_device_name(const BlockDriverState *bs)
     return bs->blk ? blk_name(bs->blk) : "";
 }
 
+const char *bdrv_get_device_or_node_name(const BlockDriverState *bs)
+{
+    return bs->blk ? blk_name(bs->blk) : bs->node_name;
+}
+
 int bdrv_get_flags(BlockDriverState *bs)
 {
     return bs->open_flags;
diff --git a/block/quorum.c b/block/quorum.c
index 437b122..f91ef75 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -226,10 +226,7 @@ static void quorum_report_bad(QuorumAIOCB *acb, char *node_name, int ret)
 
 static void quorum_report_failure(QuorumAIOCB *acb)
 {
-    const char *reference = bdrv_get_device_name(acb->common.bs)[0] ?
-                            bdrv_get_device_name(acb->common.bs) :
-                            acb->common.bs->node_name;
-
+    const char *reference = bdrv_get_device_or_node_name(acb->common.bs);
     qapi_event_send_quorum_failure(reference, acb->sector_num,
                                    acb->nb_sectors, &error_abort);
 }
diff --git a/include/block/block.h b/include/block/block.h
index 4c57d63..b285e0d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -398,6 +398,7 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
                          void *opaque);
 const char *bdrv_get_node_name(const BlockDriverState *bs);
 const char *bdrv_get_device_name(const BlockDriverState *bs);
+const char *bdrv_get_device_or_node_name(const BlockDriverState *bs);
 int bdrv_get_flags(BlockDriverState *bs);
 int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
                           const uint8_t *buf, int nb_sectors);
-- 
2.1.4

[Qemu-devel] [PATCH 2/3] block: use bdrv_get_device_or_node_name() in error messages

[Alberto Garcia]

There are several error messages that identify a BlockDriverState by
its device name. However those errors can be produced in nodes that
don't have a device name associated.

In those cases we should use bdrv_get_device_or_node_name() to fall
back to the node name and produce a more meaningful message.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c       | 13 +++++++------
 block/qcow.c  |  4 ++--
 block/qcow2.c |  2 +-
 block/qed.c   |  2 +-
 block/vdi.c   |  2 +-
 block/vhdx.c  |  2 +-
 block/vmdk.c  |  4 ++--
 block/vpc.c   |  2 +-
 block/vvfat.c |  3 ++-
 9 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index af284e3..655d9aa 100644
--- a/block.c
+++ b/block.c
@@ -1225,7 +1225,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
     } else if (backing_hd) {
         error_setg(&bs->backing_blocker,
                    "device is used as backing hd of '%s'",
-                   bdrv_get_device_name(bs));
+                   bdrv_get_device_or_node_name(bs));
     }
 
     bs->backing_hd = backing_hd;
@@ -1813,7 +1813,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
     if (!(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR) &&
         reopen_state->flags & BDRV_O_RDWR) {
         error_set(errp, QERR_DEVICE_IS_READ_ONLY,
-                  bdrv_get_device_name(reopen_state->bs));
+                  bdrv_get_device_or_node_name(reopen_state->bs));
         goto error;
     }
 
@@ -1840,7 +1840,8 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         /* It is currently mandatory to have a bdrv_reopen_prepare()
          * handler for each supported drv. */
         error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
-                  drv->format_name, bdrv_get_device_name(reopen_state->bs),
+                  drv->format_name,
+                  bdrv_get_device_or_node_name(reopen_state->bs),
                  "reopening of file");
         ret = -1;
         goto error;
@@ -3765,7 +3766,7 @@ void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp)
     if (key) {
         if (!bdrv_is_encrypted(bs)) {
             error_setg(errp, "Device '%s' is not encrypted",
-                      bdrv_get_device_name(bs));
+                      bdrv_get_device_or_node_name(bs));
         } else if (bdrv_set_key(bs, key) < 0) {
             error_set(errp, QERR_INVALID_PASSWORD);
         }
@@ -3773,7 +3774,7 @@ void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp)
         if (bdrv_key_required(bs)) {
             error_set(errp, ERROR_CLASS_DEVICE_ENCRYPTED,
                       "'%s' (%s) is encrypted",
-                      bdrv_get_device_name(bs),
+                      bdrv_get_device_or_node_name(bs),
                       bdrv_get_encrypted_filename(bs));
         }
     }
@@ -5545,7 +5546,7 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
         blocker = QLIST_FIRST(&bs->op_blockers[op]);
         if (errp) {
             error_setg(errp, "Device '%s' is busy: %s",
-                       bdrv_get_device_name(bs),
+                       bdrv_get_device_or_node_name(bs),
                        error_get_pretty(blocker->reason));
         }
         return true;
diff --git a/block/qcow.c b/block/qcow.c
index 0558969..d4287ca 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -124,7 +124,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
         snprintf(version, sizeof(version), "QCOW version %" PRIu32,
                  header.version);
         error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
-                  bdrv_get_device_name(bs), "qcow", version);
+                  bdrv_get_device_or_node_name(bs), "qcow", version);
         ret = -ENOTSUP;
         goto fail;
     }
@@ -231,7 +231,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
     /* Disable migration when qcow images are used */
     error_set(&s->migration_blocker,
               QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
-              "qcow", bdrv_get_device_name(bs), "live migration");
+              "qcow", bdrv_get_device_or_node_name(bs), "live migration");
     migrate_add_blocker(s->migration_blocker);
 
     qemu_co_mutex_init(&s->lock);
diff --git a/block/qcow2.c b/block/qcow2.c
index 32bdf75..168006b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -207,7 +207,7 @@ static void GCC_FMT_ATTR(3, 4) report_unsupported(BlockDriverState *bs,
     va_end(ap);
 
     error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
-              bdrv_get_device_name(bs), "qcow2", msg);
+              bdrv_get_device_or_node_name(bs), "qcow2", msg);
 }
 
 static void report_unsupported_feature(BlockDriverState *bs,
diff --git a/block/qed.c b/block/qed.c
index 892b13c..7d32ce4 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -408,7 +408,7 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
         snprintf(buf, sizeof(buf), "%" PRIx64,
             s->header.features & ~QED_FEATURE_MASK);
         error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
-            bdrv_get_device_name(bs), "QED", buf);
+            bdrv_get_device_or_node_name(bs), "QED", buf);
         return -ENOTSUP;
     }
     if (!qed_is_cluster_size_valid(s->header.cluster_size)) {
diff --git a/block/vdi.c b/block/vdi.c
index 53bd02f..f0dc364 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -504,7 +504,7 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
     /* Disable migration when vdi images are used */
     error_set(&s->migration_blocker,
               QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
-              "vdi", bdrv_get_device_name(bs), "live migration");
+              "vdi", bdrv_get_device_or_node_name(bs), "live migration");
     migrate_add_blocker(s->migration_blocker);
 
     qemu_co_mutex_init(&s->write_lock);
diff --git a/block/vhdx.c b/block/vhdx.c
index bb3ed45..d5dfe00 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1004,7 +1004,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
     /* Disable migration when VHDX images are used */
     error_set(&s->migration_blocker,
             QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
-            "vhdx", bdrv_get_device_name(bs), "live migration");
+            "vhdx", bdrv_get_device_or_node_name(bs), "live migration");
     migrate_add_blocker(s->migration_blocker);
 
     return 0;
diff --git a/block/vmdk.c b/block/vmdk.c
index 8410a15..0230070 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -669,7 +669,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
         snprintf(buf, sizeof(buf), "VMDK version %" PRId32,
                  le32_to_cpu(header.version));
         error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
-                  bdrv_get_device_name(bs), "vmdk", buf);
+                  bdrv_get_device_or_node_name(bs), "vmdk", buf);
         return -ENOTSUP;
     } else if (le32_to_cpu(header.version) == 3 && (flags & BDRV_O_RDWR)) {
         /* VMware KB 2064959 explains that version 3 added support for
@@ -964,7 +964,7 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
     /* Disable migration when VMDK images are used */
     error_set(&s->migration_blocker,
               QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
-              "vmdk", bdrv_get_device_name(bs), "live migration");
+              "vmdk", bdrv_get_device_or_node_name(bs), "live migration");
     migrate_add_blocker(s->migration_blocker);
     g_free(buf);
     return 0;
diff --git a/block/vpc.c b/block/vpc.c
index 43e768e..fd418b9 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -320,7 +320,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
     /* Disable migration when VHD images are used */
     error_set(&s->migration_blocker,
               QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
-              "vpc", bdrv_get_device_name(bs), "live migration");
+              "vpc", bdrv_get_device_or_node_name(bs), "live migration");
     migrate_add_blocker(s->migration_blocker);
 
     return 0;
diff --git a/block/vvfat.c b/block/vvfat.c
index 9be632f..9ed3291 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1182,7 +1182,8 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
     if (s->qcow) {
         error_set(&s->migration_blocker,
                   QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
-                  "vvfat (rw)", bdrv_get_device_name(bs), "live migration");
+                  "vvfat (rw)", bdrv_get_device_or_node_name(bs),
+                  "live migration");
         migrate_add_blocker(s->migration_blocker);
     }
 
-- 
2.1.4

[Qemu-devel] [PATCH 3/3] block: allow BLOCK_IMAGE_CORRUPTED to have a node name

[Alberto Garcia]

Since this event can occur in nodes that don't have a device name
associated, use the node name as fallback in those cases.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c           | 5 +++--
 docs/qmp/qmp-events.txt | 2 +-
 qapi/block-core.json    | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 168006b..d808c70 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2832,8 +2832,9 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
                 "corruption events will be suppressed\n", message);
     }
 
-    qapi_event_send_block_image_corrupted(bdrv_get_device_name(bs), message,
-                                          offset >= 0, offset, size >= 0, size,
+    qapi_event_send_block_image_corrupted(bdrv_get_device_or_node_name(bs),
+                                          message, offset >= 0, offset,
+                                          size >= 0, size,
                                           fatal, &error_abort);
     g_free(message);
 
diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
index d759d19..75f3e68 100644
--- a/docs/qmp/qmp-events.txt
+++ b/docs/qmp/qmp-events.txt
@@ -35,7 +35,7 @@ Emitted when a disk image is being marked corrupt.
 
 Data:
 
-- "device": Device name (json-string)
+- "device": Device name, or node name if not present (json-string)
 - "msg":    Informative message (e.g., reason for the corruption) (json-string)
 - "offset": If the corruption resulted from an image access, this is the access
             offset into the image (json-int)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 42c8850..3b51c68 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1751,7 +1751,7 @@
 #
 # Emitted when a corruption has been detected in a disk image
 #
-# @device: device name
+# @device: device name, or node name if not present
 #
 # @msg: informative message for human consumption, such as the kind of
 #       corruption being detected. It should not be parsed by machine as it is
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/3] block: add bdrv_get_device_or_node_name()

[Max Reitz]

On 2015-03-19 at 11:43, Alberto Garcia wrote:
> This function gets the device name associated with a BlockDriverState,
> or its node name if the device name is empty.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   block.c               | 5 +++++
>   block/quorum.c        | 5 +----
>   include/block/block.h | 1 +
>   3 files changed, 7 insertions(+), 4 deletions(-)

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

Re: [Qemu-devel] [PATCH 2/3] block: use bdrv_get_device_or_node_name() in error messages

[Max Reitz]

On 2015-03-19 at 11:43, Alberto Garcia wrote:
> There are several error messages that identify a BlockDriverState by
> its device name. However those errors can be produced in nodes that
> don't have a device name associated.
>
> In those cases we should use bdrv_get_device_or_node_name() to fall
> back to the node name and produce a more meaningful message.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   block.c       | 13 +++++++------
>   block/qcow.c  |  4 ++--
>   block/qcow2.c |  2 +-
>   block/qed.c   |  2 +-
>   block/vdi.c   |  2 +-
>   block/vhdx.c  |  2 +-
>   block/vmdk.c  |  4 ++--
>   block/vpc.c   |  2 +-
>   block/vvfat.c |  3 ++-
>   9 files changed, 18 insertions(+), 16 deletions(-)

Well, it may pose a problem that the error messages often state "Device 
'%s'", but with this change, it's not always a device, but sometimes 
just a node. Maybe it would be better to replace the "Device" in the 
error messages by "Node" (a node can be specified both by the node name 
and the device name, whereas a device can only be referenced by its 
device name).

Apart from this, the idea of this change looks good, though, as do the 
changes done by this patch.

Max

Re: [Qemu-devel] [PATCH 0/3] Add bdrv_get_device_or_node_name()

[Max Reitz]

On 2015-03-19 at 11:43, Alberto Garcia wrote:
> We have several cases of error messages related to BlockDriverState that expect it to have a device name.
>
> However many of those can happen in nodes that don't have a device name associated so the generated error messages are not very meaningful.
>
> This series adds a new function bdrv_get_device_or_node_name(), that returns the node name if there is no device name for that node. Since both the device and the node name live in the same namespace, there's no ambiguity. And since it gives preference to the device name, it maintains backward compatibility.

Not necessarily. Imagine a program interpreting the 
BLOCK_IMAGE_CORRUPTED event, for example (after this series): It sees 
the @device value and expects a device name there; then it tries to do 
something with that, assuming it is a device, but that may fail (like 
looking it up in its own list of devices, or launching a block job, or 
whatever; I can very well imagine someone wanting to automatically start 
a blockdev_backup() on that event, for instance).

Of course, this is a very weak argument. Before this series, in these 
cases the caller only received an empty string, which didn't help it 
very much either. So I just want to point out that it does not fully 
maintain compatibility, but I don't think there will be any problems 
whatsoever either.

Max

> It also updates all calls to bdrv_get_device_name() where it makes sense: several error messages and the BLOCK_IMAGE_CORRUPTED event.
>
> I wrote this thinking on the case where any arbitrary node will be able to own a block job, but I think it's useful already for the reasons explained above.
>
> Berto
>
> Alberto Garcia (3):
>    block: add bdrv_get_device_or_node_name()
>    block: use bdrv_get_device_or_node_name() in error messages
>    block: allow BLOCK_IMAGE_CORRUPTED to have a node name
>
>   block.c                 | 18 ++++++++++++------
>   block/qcow.c            |  4 ++--
>   block/qcow2.c           |  7 ++++---
>   block/qed.c             |  2 +-
>   block/quorum.c          |  5 +----
>   block/vdi.c             |  2 +-
>   block/vhdx.c            |  2 +-
>   block/vmdk.c            |  4 ++--
>   block/vpc.c             |  2 +-
>   block/vvfat.c           |  3 ++-
>   docs/qmp/qmp-events.txt |  2 +-
>   include/block/block.h   |  1 +
>   qapi/block-core.json    |  2 +-
>   13 files changed, 30 insertions(+), 24 deletions(-)
>

Re: [Qemu-devel] [PATCH 3/3] block: allow BLOCK_IMAGE_CORRUPTED to have a node name

[Max Reitz]

On 2015-03-19 at 11:43, Alberto Garcia wrote:
> Since this event can occur in nodes that don't have a device name
> associated, use the node name as fallback in those cases.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   block/qcow2.c           | 5 +++--
>   docs/qmp/qmp-events.txt | 2 +-
>   qapi/block-core.json    | 2 +-
>   3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 168006b..d808c70 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2832,8 +2832,9 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
>                   "corruption events will be suppressed\n", message);
>       }
>   
> -    qapi_event_send_block_image_corrupted(bdrv_get_device_name(bs), message,
> -                                          offset >= 0, offset, size >= 0, size,
> +    qapi_event_send_block_image_corrupted(bdrv_get_device_or_node_name(bs),
> +                                          message, offset >= 0, offset,
> +                                          size >= 0, size,
>                                             fatal, &error_abort);
>       g_free(message);
>   
> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
> index d759d19..75f3e68 100644
> --- a/docs/qmp/qmp-events.txt
> +++ b/docs/qmp/qmp-events.txt
> @@ -35,7 +35,7 @@ Emitted when a disk image is being marked corrupt.
>   
>   Data:
>   
> -- "device": Device name (json-string)
> +- "device": Device name, or node name if not present (json-string)
>   - "msg":    Informative message (e.g., reason for the corruption) (json-string)
>   - "offset": If the corruption resulted from an image access, this is the access
>               offset into the image (json-int)
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 42c8850..3b51c68 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1751,7 +1751,7 @@
>   #
>   # Emitted when a corruption has been detected in a disk image
>   #
> -# @device: device name
> +# @device: device name, or node name if not present
>   #
>   # @msg: informative message for human consumption, such as the kind of
>   #       corruption being detected. It should not be parsed by machine as it is

Basically the same as my reply to patch 2, but here it's a formal 
question as well: Normally, if a field in QMP is designed @device, it 
contains a device name. We do have combined device/node name fields, 
though (as of John's incremental backup series, at least), but those are 
named @node (which I proposed for patch 2, too).

But renaming the field here will lead to breaking backwards 
compatibility. I think just adding a @node-name field and keeping 
@device as it is should be good enough here.

Max

Re: [Qemu-devel] [PATCH 3/3] block: allow BLOCK_IMAGE_CORRUPTED to have a node name

[Alberto Garcia]

(I forgot to Cc Eric in this series, doing it now)

On Thu, Mar 19, 2015 at 03:42:35PM -0400, Max Reitz wrote:
> >  # Emitted when a corruption has been detected in a disk image
> >  #
> >-# @device: device name
> >+# @device: device name, or node name if not present
> 
> Normally, if a field in QMP is designed @device, it contains a
> device name. We do have combined device/node name fields, though (as
> of John's incremental backup series, at least), but those are named
> @node (which I proposed for patch 2, too).
> 
> But renaming the field here will lead to breaking backwards
> compatibility. I think just adding a @node-name field and keeping
> @device as it is should be good enough here.

I was doing the same that we discussed for BlockJobInfo here, where
option b) seemed to have a bit more support:

https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03651.html

But yeah I personally don't mind extending the event with a new field.
Would we make 'device' optional in this case?

Berto

Re: [Qemu-devel] [PATCH 0/3] Add bdrv_get_device_or_node_name()

[Alberto Garcia]

On Thu, Mar 19, 2015 at 03:37:13PM -0400, Max Reitz wrote:

> > This series adds a new function bdrv_get_device_or_node_name(),
> > that returns the node name if there is no device name for that
> > node. Since both the device and the node name live in the same
> > namespace, there's no ambiguity. And since it gives preference to
> > the device name, it maintains backward compatibility.
> 
> Not necessarily. Imagine a program interpreting the
> BLOCK_IMAGE_CORRUPTED event, for example (after this series): It
> sees the @device value and expects a device name there; then it
> tries to do something with that, assuming it is a device, but
> that may fail (like looking it up in its own list of devices, or
> launching a block job, or whatever; I can very well imagine someone
> wanting to automatically start a blockdev_backup() on that event,
> for instance).
> 
> Of course, this is a very weak argument. Before this series, in
> these cases the caller only received an empty string, which didn't
> help it very much either. So I just want to point out that it does
> not fully maintain compatibility, but I don't think there will be
> any problems whatsoever either.

The idea is that if some software can handle the return data of some
operation, it should be able to continue doing so. So we return device
names in the same cases where we used to return device names.

The cases where we are returning different data now are cases that
older software probably doesn't know how to handle anyway.

But yeah, you're right. Having better compatibility would require us
to add a different field for the node name. But this didn't seem to be
much of a concern for Eric, at least in the BlockJobInfo case.

Berto

Re: [Qemu-devel] [PATCH 3/3] block: allow BLOCK_IMAGE_CORRUPTED to have a node name

[Max Reitz]

On 2015-03-19 at 17:42, Alberto Garcia wrote:
> (I forgot to Cc Eric in this series, doing it now)
>
> On Thu, Mar 19, 2015 at 03:42:35PM -0400, Max Reitz wrote:
>>>   # Emitted when a corruption has been detected in a disk image
>>>   #
>>> -# @device: device name
>>> +# @device: device name, or node name if not present
>> Normally, if a field in QMP is designed @device, it contains a
>> device name. We do have combined device/node name fields, though (as
>> of John's incremental backup series, at least), but those are named
>> @node (which I proposed for patch 2, too).
>>
>> But renaming the field here will lead to breaking backwards
>> compatibility. I think just adding a @node-name field and keeping
>> @device as it is should be good enough here.
> I was doing the same that we discussed for BlockJobInfo here, where
> option b) seemed to have a bit more support:
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03651.html
>
> But yeah I personally don't mind extending the event with a new field.
> Would we make 'device' optional in this case?

No, I think we'd need to keep it. It isn't optional right now so any 
software using the monitor will expect it to be present (even if it's 
empty).

Regarding the BlockJobInfo discussion: One argument I can see there is 
"Particularly if we don't have two parameters for starting the job, then 
we don't need two parameters for reporting it"/"If you're going to reuse 
'device' on the creation, then reuse it on the reporting", which does 
not apply here (there is no command corresponding to this event, it just 
pops up on its own), so there will not be any asymmetry here.

Other than that, the only argument I can see is "it will work with 
libvirt, so it is fine", but that's not really a reason to prefer b) 
over a)...

So in this case here I don't really see a benefit of reusing @device 
instead of just adding @node-name, whereas adding @node-name will have 
the benefit of not affecting anybody and (in my opinion) being more 
explicit. However, if others tend to think otherwise (the @node-name vs. 
@node vs. @device is a constant point of dissent over naming...), I'm 
happy to be convinced otherwise. In the end it doesn't really matter 
after all, it's a machine-readable protocol. If software can work with 
it, it's fine.

Max

Re: [Qemu-devel] [PATCH 3/3] block: allow BLOCK_IMAGE_CORRUPTED to have a node name

[Alberto Garcia]

On Thu, Mar 19, 2015 at 05:52:52PM -0400, Max Reitz wrote:

> So in this case here I don't really see a benefit of reusing @device
> instead of just adding @node-name, whereas adding @node-name will
> have the benefit of not affecting anybody and (in my opinion) being
> more explicit. However, if others tend to think otherwise (the
> @node-name vs. @node vs. @device is a constant point of dissent
> over naming...), I'm happy to be convinced otherwise. In the end it
> doesn't really matter after all, it's a machine-readable protocol.
> If software can work with it, it's fine.

It's fine with me as well, I just had the impression that there was a
consensus towards not extending the API if possible, but in my case
I'm happy to implement whichever alternative.

Berto

Re: [Qemu-devel] [PATCH 3/3] block: allow BLOCK_IMAGE_CORRUPTED to have a node name

[Eric Blake]

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

On 03/19/2015 03:42 PM, Alberto Garcia wrote:
> (I forgot to Cc Eric in this series, doing it now)
> 
> On Thu, Mar 19, 2015 at 03:42:35PM -0400, Max Reitz wrote:
>>>  # Emitted when a corruption has been detected in a disk image
>>>  #
>>> -# @device: device name
>>> +# @device: device name, or node name if not present
>>
>> Normally, if a field in QMP is designed @device, it contains a
>> device name. We do have combined device/node name fields, though (as
>> of John's incremental backup series, at least), but those are named
>> @node (which I proposed for patch 2, too).
>>
>> But renaming the field here will lead to breaking backwards
>> compatibility. I think just adding a @node-name field and keeping
>> @device as it is should be good enough here.
> 
> I was doing the same that we discussed for BlockJobInfo here, where
> option b) seemed to have a bit more support:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03651.html
> 
> But yeah I personally don't mind extending the event with a new field.
> Would we make 'device' optional in this case?

How hard is it to output both 'device' and 'node' in the same event, if
both are available?  And does it add anything?  I could live with the
simplicity of just returning a device name where we have one
(back-compat for older clients that only ever start jobs on a device)
and a node name where we don't (such an event can only be triggered by a
job started by a client new enough to know how to start a job by node
name), and just always use the 'device' field while documenting that it
is not the best field name for what its contents represent.

On the other hand, if libvirt starts using node names everywhere, then
returning a device name instead of a node name makes libvirt have to do
a bit more work to map a device name down to a node name (not the end of
the world, because the device name is still unambiguous); whereas
returning both device AND node name at once may make libvirt's life easier.

And for this particular event, which is not tied to block jobs but to
generic block operation, isn't it possible that we could be reporting a
corrupted backing chain where we have neither a device name (it is not
the active layer) nor a node name (if we don't add Jeff's patch to
auto-name all nodes)?  In such a case, I don't know that we can do much
better anyways.

-- 
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/3] block: allow BLOCK_IMAGE_CORRUPTED to have a node name

[Alberto Garcia]

On Thu, Mar 19, 2015 at 04:15:49PM -0600, Eric Blake wrote:

> >>> -# @device: device name
> >>> +# @device: device name, or node name if not present
> >>
> >> I think just adding a @node-name field and keeping @device as it
> >> is should be good enough here.
> > 
> > I was doing the same that we discussed for BlockJobInfo here, where
> > option b) seemed to have a bit more support:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03651.html
> > 
> > But yeah I personally don't mind extending the event with a new field.
> > Would we make 'device' optional in this case?
> 
> How hard is it to output both 'device' and 'node' in the same event,
> if both are available?

It's a trivial change, there's no problem at all. I assume that, for
compatibility reasons, 'device' would continue to be present even if
it's empty, but would you prefer to have 'node-name' as an optional
field?

> And for this particular event, which is not tied to block jobs but
> to generic block operation, isn't it possible that we could be
> reporting a corrupted backing chain where we have neither a device
> name (it is not the active layer) nor a node name (if we don't add
> Jeff's patch to auto-name all nodes)?  In such a case, I don't know
> that we can do much better anyways.

Yes, it is perfectly possible. I guess any software that wants to
handle those scenarios probably wants to give names to all nodes.

From the QEMU side, apart from giving automatic names to all nodes I
don't see any other solution.

Berto

Re: [Qemu-devel] [PATCH 3/3] block: allow BLOCK_IMAGE_CORRUPTED to have a node name

[Eric Blake]

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

On 03/19/2015 04:38 PM, Alberto Garcia wrote:

>> And for this particular event, which is not tied to block jobs but
>> to generic block operation, isn't it possible that we could be
>> reporting a corrupted backing chain where we have neither a device
>> name (it is not the active layer) nor a node name (if we don't add
>> Jeff's patch to auto-name all nodes)?  In such a case, I don't know
>> that we can do much better anyways.
> 
> Yes, it is perfectly possible. I guess any software that wants to
> handle those scenarios probably wants to give names to all nodes.
> 
> From the QEMU side, apart from giving automatic names to all nodes I
> don't see any other solution.

In the context of block commit, libvirt found that reporting device name
to the end user was nicer than file name (since there are two events
emitted - one when the active layer has become synced with the lower
layer, and therefore where the node associated with device is still
'top'; and the other when the active layer has pivoted to the base layer
and top is no longer valid, therefore where the node associated with
device is now 'base' - since the two events are related but have
different file [node] names, having the device name made life easier).
But note that we still haven't exposed 'node' information in any of the
block job events (BLOCK_JOB_COMPLETED, BLOCK_JOB_CANCELLED,
BLOCK_JOB_ERROR), and it might still make sense to do so, even if
libvirt doesn't need to expose those node names on to the end user.

But this is a counter-example - knowing just the device name does NOT
tell you which node is corrupted, if there is any possibility of a
snapshot or active commit changing the active node associated with the
device.  That's because there might be a race between the event
announcing corruption and some other action modifying the chain, such
that the client's notion of the active element of the chain is different
from the active element at the time the event was reported.

So the more I think about it, the more I'd like for this event to output
both 'device' (mandatory, with an empty string if we can't easily tie
the BDS to a single device) and 'node' (where 'node' can be optional,
and omitted if the BDS does not have a node name [if we ever add
generated node names, then we could make 'node' mandatory at that time]).

-- 
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 1/3] block: add bdrv_get_device_or_node_name()

[Markus Armbruster]

Alberto Garcia <berto@igalia.com> writes:

> This function gets the device name associated with a BlockDriverState,
> or its node name if the device name is empty.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c               | 5 +++++
>  block/quorum.c        | 5 +----
>  include/block/block.h | 1 +
>  3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/block.c b/block.c
> index 0fe97de..af284e3 100644
> --- a/block.c
> +++ b/block.c
> @@ -3920,6 +3920,11 @@ const char *bdrv_get_device_name(const BlockDriverState *bs)
>      return bs->blk ? blk_name(bs->blk) : "";
>  }
>  
> +const char *bdrv_get_device_or_node_name(const BlockDriverState *bs)
> +{
> +    return bs->blk ? blk_name(bs->blk) : bs->node_name;
> +}
> +

Does this have uses beyond identifying @bs to the user?

>  int bdrv_get_flags(BlockDriverState *bs)
>  {
>      return bs->open_flags;
> diff --git a/block/quorum.c b/block/quorum.c
> index 437b122..f91ef75 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -226,10 +226,7 @@ static void quorum_report_bad(QuorumAIOCB *acb, char *node_name, int ret)
>  
>  static void quorum_report_failure(QuorumAIOCB *acb)
>  {
> -    const char *reference = bdrv_get_device_name(acb->common.bs)[0] ?
> -                            bdrv_get_device_name(acb->common.bs) :
> -                            acb->common.bs->node_name;
> -
> +    const char *reference = bdrv_get_device_or_node_name(acb->common.bs);
>      qapi_event_send_quorum_failure(reference, acb->sector_num,
>                                     acb->nb_sectors, &error_abort);
>  }

Preexisting: what if reference is null?

event.json suggests it can't be null:

    ##
    # @QUORUM_FAILURE
    #
    # Emitted by the Quorum block driver if it fails to establish a quorum
    #
    # @reference: device name if defined else node name
    #
    # @sector-num: number of the first sector of the failed read operation
    #
    # @sectors-count: failed read operation sector count
    #
    # Since: 2.0
    ##
    { 'event': 'QUORUM_FAILURE',
      'data': { 'reference': 'str', 'sector-num': 'int', 'sectors-count': 'int' } }

> diff --git a/include/block/block.h b/include/block/block.h
> index 4c57d63..b285e0d 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -398,6 +398,7 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
>                           void *opaque);
>  const char *bdrv_get_node_name(const BlockDriverState *bs);
>  const char *bdrv_get_device_name(const BlockDriverState *bs);
> +const char *bdrv_get_device_or_node_name(const BlockDriverState *bs);
>  int bdrv_get_flags(BlockDriverState *bs);
>  int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>                            const uint8_t *buf, int nb_sectors);

Re: [Qemu-devel] [PATCH 2/3] block: use bdrv_get_device_or_node_name() in error messages

[Markus Armbruster]

Alberto Garcia <berto@igalia.com> writes:

> There are several error messages that identify a BlockDriverState by
> its device name. However those errors can be produced in nodes that
> don't have a device name associated.
>
> In those cases we should use bdrv_get_device_or_node_name() to fall
> back to the node name and produce a more meaningful message.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c       | 13 +++++++------
>  block/qcow.c  |  4 ++--
>  block/qcow2.c |  2 +-
>  block/qed.c   |  2 +-
>  block/vdi.c   |  2 +-
>  block/vhdx.c  |  2 +-
>  block/vmdk.c  |  4 ++--
>  block/vpc.c   |  2 +-
>  block/vvfat.c |  3 ++-
>  9 files changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/block.c b/block.c
> index af284e3..655d9aa 100644
> --- a/block.c
> +++ b/block.c
> @@ -1225,7 +1225,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>      } else if (backing_hd) {
>          error_setg(&bs->backing_blocker,
>                     "device is used as backing hd of '%s'",
> -                   bdrv_get_device_name(bs));
> +                   bdrv_get_device_or_node_name(bs));
>      }
>  
>      bs->backing_hd = backing_hd;

Before:

* if @bs belongs to a BB with name N, print "backing hd of 'N'"
* else print "backing hd of ''" (not nice, but works)

After: 

* if @bs belongs to a BB with name N, print "backing hd of 'N'" (no change)
* else if @bs has a node name NN, print "backing hd of 'NN'" (improvement)
* else print a null pointer, which crashes on some systems (bug)

If you want your bdrv_get_device_or_node_name() serve as drop-in
replacement for bdrv_get_device_name(), it must not return null.

In my opinion, such a drop-in replacement can only be a stop gap.  We
need to review every error message and figure out how to update it for
node names.  We should've done it back when we added node names.

Your misuse of bdrv_get_device_or_node_name() shows misuse is likely.
As long as that's the case, it needs a function comment.

[...]

Re: [Qemu-devel] [PATCH 1/3] block: add bdrv_get_device_or_node_name()

[Alberto Garcia]

On Fri, Mar 20, 2015 at 08:40:32AM +0100, Markus Armbruster wrote:

> > +const char *bdrv_get_device_or_node_name(const BlockDriverState *bs)
> > +{
> > +    return bs->blk ? blk_name(bs->blk) : bs->node_name;
> > +}
> > +
> 
> Does this have uses beyond identifying @bs to the user?

None that I can think of, although apart from error messages it can
also be used in data types, like the cases of BLOCK_IMAGE_CORRUPTED
and BlockJobInfo that are being discussed:

   https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03651.html

> >  static void quorum_report_failure(QuorumAIOCB *acb)
> >  {
> > -    const char *reference = bdrv_get_device_name(acb->common.bs)[0] ?
> > -                            bdrv_get_device_name(acb->common.bs) :
> > -                            acb->common.bs->node_name;
> > -
> > +    const char *reference = bdrv_get_device_or_node_name(acb->common.bs);
> >      qapi_event_send_quorum_failure(reference, acb->sector_num,
> >                                     acb->nb_sectors, &error_abort);
> >  }
> 
> Preexisting: what if reference is null?

It can't happen, both the device and the node name strings are
guaranteed to be non-null. The latter is actually a static string so
there's no chance bs->node_name returns a null pointer.

Berto

Re: [Qemu-devel] [PATCH 1/3] block: add bdrv_get_device_or_node_name()

[Markus Armbruster]

Alberto Garcia <berto@igalia.com> writes:

> On Fri, Mar 20, 2015 at 08:40:32AM +0100, Markus Armbruster wrote:
>
>> > +const char *bdrv_get_device_or_node_name(const BlockDriverState *bs)
>> > +{
>> > +    return bs->blk ? blk_name(bs->blk) : bs->node_name;
>> > +}
>> > +
>> 
>> Does this have uses beyond identifying @bs to the user?
>
> None that I can think of, although apart from error messages it can
> also be used in data types, like the cases of BLOCK_IMAGE_CORRUPTED
> and BlockJobInfo that are being discussed:
>
>    https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg03651.html

Workable because BB and BDS names share a common name space.  But is it
a good idea?  No need to discuss this here if it's already discussed
elsewhere.

>> >  static void quorum_report_failure(QuorumAIOCB *acb)
>> >  {
>> > -    const char *reference = bdrv_get_device_name(acb->common.bs)[0] ?
>> > -                            bdrv_get_device_name(acb->common.bs) :
>> > -                            acb->common.bs->node_name;
>> > -
>> > +    const char *reference = bdrv_get_device_or_node_name(acb->common.bs);
>> >      qapi_event_send_quorum_failure(reference, acb->sector_num,
>> >                                     acb->nb_sectors, &error_abort);
>> >  }
>> 
>> Preexisting: what if reference is null?
>
> It can't happen, both the device and the node name strings are
> guaranteed to be non-null. The latter is actually a static string so
> there's no chance bs->node_name returns a null pointer.

You're right, I missed the fact that BDS member node_name is an array.

Suggest to add a function comment to bdrv_get_device_or_node_name()
explaining intended use, and that it never returns null.

Re: [Qemu-devel] [PATCH 3/3] block: allow BLOCK_IMAGE_CORRUPTED to have a node name

[Alberto Garcia]

On Thu, Mar 19, 2015 at 05:14:24PM -0600, Eric Blake wrote:

> So the more I think about it, the more I'd like for this event to
> output both 'device' (mandatory, with an empty string if we can't
> easily tie the BDS to a single device) and 'node' (where 'node' can
> be optional, and omitted if the BDS does not have a node name [if we
> ever add generated node names, then we could make 'node' mandatory
> at that time]).

Ok, I will change that.

On a related note, the QUORUM_FAILURE event does have this mixed field
already, called "reference", which is the device name if present, else
the node name.

But I'm not planning to change it unless you think it's a good idea.
That would be a thing for a different patch anyway.

Berto