Add bdrv_get_device_or_node_name()

PATCH Superseded Alberto Garcia 3 participants 10 messages Mar 20, 2015 – Mar 23, 2015 Block layer

lore

Share & tools

lore

Alberto Garcia

author

2015-03-20 14:34 UTC #0 lore raw

v3 cover Add bdrv_get_device_or_node_name()

One more try.

v3:
- The node-name field in BLOCK_IMAGE_CORRUPTED is now Since: 2.4
- Remove the QERR_ macros instead of updating them. The text message is adapted to each case where applicable, and 'device' is renamed to 'node' only where it makes sense.

v2:
- bdrv_get_device_or_node_name() includes a comment explaining its usage.
- The error messages have been updated to say 'node' instead of 'device' where appropriate.
- The BLOCK_IMAGE_CORRUPTED event has a new 'node-name' field.

Regards,

Berto

Alberto Garcia (3):
block: add bdrv_get_device_or_node_name()
block: use bdrv_get_device_or_node_name() in error messages
block: add 'node-name' field to BLOCK_IMAGE_CORRUPTED

 block.c                   | 33 +++++++++++++++++++++------------
 block/qcow.c              |  8 ++++----
 block/qcow2.c             | 10 +++++++---
 block/qed.c               |  2 +-
 block/quorum.c            |  5 +----
 block/snapshot.c          | 12 ++++++------
 block/vdi.c               |  6 +++---
 block/vhdx.c              |  6 +++---
 block/vmdk.c              |  8 ++++----
 block/vpc.c               |  6 +++---
 block/vvfat.c             |  7 ++++---
 blockdev.c                |  9 +++++----
 docs/qmp/qmp-events.txt   | 16 +++++++++-------
 include/block/block.h     |  1 +
 include/qapi/qmp/qerror.h |  6 ------
 qapi/block-core.json      | 13 ++++++++-----
 16 files changed, 80 insertions(+), 68 deletions(-)

Signature · 1 line

2.1.4

Alberto Garcia

author

2015-03-20 14:34 UTC #1 lore raw

PATCH v3 1/3 block: add bdrv_get_device_or_node_name()

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 <[email protected]>

---
 block.c               | 9 +++++++++
 block/quorum.c        | 5 +----
 include/block/block.h | 1 +
 3 files changed, 11 insertions(+), 4 deletions(-)

block.c +9 -0

index 0fe97de..98b90b4 100644
@@ -3920,6 +3920,15 @@ const char *bdrv_get_device_name(const BlockDriverState *bs)
     return bs->blk ? blk_name(bs->blk) : "";
 }
 
+/* This can be used to identify nodes that might not have a device
+ * name associated. Since node and device names live in the same
+ * namespace, the result is unambiguous. The exception is if both are
+ * absent, then this returns an empty (non-null) string. */
+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;

block/quorum.c +1 -4

index 437b122..f91ef75 100644
@@ -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);
 }

include/block/block.h +1 -0

index 4c57d63..b285e0d 100644
@@ -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);

Signature · 1 line

2.1.4

Alberto Garcia

author

2015-03-20 14:34 UTC #5 lore raw

PATCH v3 3/3 block: add 'node-name' field to BLOCK_IMAGE_CORRUPTED

In reply to Alberto Garcia

Since this event can occur in nodes that cannot have a device name associated, include also a field with the node name.

Signed-off-by Alberto Garcia <[email protected]>

---
 block/qcow2.c           |  8 ++++++--
 docs/qmp/qmp-events.txt | 16 +++++++++-------
 qapi/block-core.json    | 13 ++++++++-----
 3 files changed, 23 insertions(+), 14 deletions(-)

block/qcow2.c +6 -2

index 168006b..e7c78f1 100644
@@ -2809,6 +2809,7 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
                              int64_t size, const char *message_format, ...)
 {
     BDRVQcowState *s = bs->opaque;
+    const char *node_name;
     char *message;
     va_list ap;

@@ -2832,8 +2833,11 @@ 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,
+    node_name = bdrv_get_node_name(bs);
+    qapi_event_send_block_image_corrupted(bdrv_get_device_name(bs),
+                                          *node_name != '\0', node_name,
+                                          message, offset >= 0, offset,
+                                          size >= 0, size,
                                           fatal, &error_abort);
     g_free(message);

docs/qmp/qmp-events.txt +9 -7

index d759d19..ed1d0a5 100644
@@ -35,17 +35,19 @@ Emitted when a disk image is being marked corrupt.
 
 Data:
 
-- "device": Device name (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)
-- "size":   If the corruption resulted from an image access, this is the access
-            size (json-int)
+- "device":    Device name (json-string)
+- "node-name": Node name, if it's 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)
+- "size":      If the corruption resulted from an image access, this
+               is the access size (json-int)
 
 Example:
 
 { "event": "BLOCK_IMAGE_CORRUPTED",
-    "data": { "device": "ide0-hd0",
+    "data": { "device": "ide0-hd0", "node-name": "node0",
         "msg": "Prevented active L1 table overwrite", "offset": 196608,
         "size": 65536 },
     "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }

qapi/block-core.json +8 -5

index f525b04..2a40b73 100644
@@ -1752,6 +1752,8 @@
 #
 # @device: device name
 #
+# @node-name: #optional node name (Since: 2.4)
+#
 # @msg: informative message for human consumption, such as the kind of
 #       corruption being detected. It should not be parsed by machine as it is
 #       not guaranteed to be stable

@@ -1769,11 +1771,12 @@
 # Since: 1.7
 ##
 { 'event': 'BLOCK_IMAGE_CORRUPTED',
-  'data': { 'device' : 'str',
-            'msg'    : 'str',
-            '*offset': 'int',
-            '*size'  : 'int',
-            'fatal'  : 'bool' } }
+  'data': { 'device'     : 'str',
+            '*node-name' : 'str',
+            'msg'        : 'str',
+            '*offset'    : 'int',
+            '*size'      : 'int',
+            'fatal'      : 'bool' } }
 
 ##
 # @BLOCK_IO_ERROR

Signature · 1 line

2.1.4

Alberto Garcia

author

2015-03-20 14:34 UTC #3 lore raw

PATCH v3 2/3 block: use bdrv_get_device_or_node_name() in error messages

In reply to 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. The messages are also updated to use the more generic term 'node' instead of 'device'.

Signed-off-by Alberto Garcia <[email protected]>

---
 block.c                   | 24 ++++++++++++------------
 block/qcow.c              |  8 ++++----
 block/qcow2.c             |  2 +-
 block/qed.c               |  2 +-
 block/snapshot.c          | 12 ++++++------
 block/vdi.c               |  6 +++---
 block/vhdx.c              |  6 +++---
 block/vmdk.c              |  8 ++++----
 block/vpc.c               |  6 +++---
 block/vvfat.c             |  7 ++++---
 blockdev.c                |  9 +++++----
 include/qapi/qmp/qerror.h |  6 ------
 12 files changed, 46 insertions(+), 50 deletions(-)

block.c +12 ~~-12~~

index 98b90b4..737ab68 100644
@@ -1224,8 +1224,8 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
         bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
     } else if (backing_hd) {
         error_setg(&bs->backing_blocker,
-                   "device is used as backing hd of '%s'",
-                   bdrv_get_device_name(bs));
+                   "node is used as backing hd of '%s'",
+                   bdrv_get_device_or_node_name(bs));
     }
 
     bs->backing_hd = backing_hd;

@@ -1812,8 +1812,8 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
      * to r/w */
     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));
+        error_setg(errp, "Node '%s' is read only",
+                   bdrv_get_device_or_node_name(reopen_state->bs));
         goto error;
     }

@@ -1839,9 +1839,9 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
     } else {
         /* 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),
-                 "reopening of file");
+        error_setg(errp, "Block format '%s' used by node '%s' "
+                   "does not support reopening files", drv->format_name,
+                   bdrv_get_device_or_node_name(reopen_state->bs));
         ret = -1;
         goto error;
     }

@@ -3764,8 +3764,8 @@ 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));
+            error_setg(errp, "Node '%s' is not encrypted",
+                      bdrv_get_device_or_node_name(bs));
         } else if (bdrv_set_key(bs, key) < 0) {
             error_set(errp, QERR_INVALID_PASSWORD);
         }

@@ -3773,7 +3773,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));
         }
     }

@@ -5548,8 +5548,8 @@ bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp)
     if (!QLIST_EMPTY(&bs->op_blockers[op])) {
         blocker = QLIST_FIRST(&bs->op_blockers[op]);
         if (errp) {
-            error_setg(errp, "Device '%s' is busy: %s",
-                       bdrv_get_device_name(bs),
+            error_setg(errp, "Node '%s' is busy: %s",
+                       bdrv_get_device_or_node_name(bs),
                        error_get_pretty(blocker->reason));
         }
         return true;

block/qcow.c +4 -4

index 0558969..ab89328 100644
@@ -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;
     }

@@ -229,9 +229,9 @@ 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");
+    error_setg(&s->migration_blocker, "The qcow format used by node '%s' "
+               "does not support live migration",
+               bdrv_get_device_or_node_name(bs));
     migrate_add_blocker(s->migration_blocker);
 
     qemu_co_mutex_init(&s->lock);

block/qcow2.c +1 -1

index 32bdf75..168006b 100644
@@ -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,

block/qed.c +1 -1

index 892b13c..7d32ce4 100644
@@ -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)) {

block/snapshot.c +6 -6

index 698e1a1..50ae610 100644
@@ -246,9 +246,9 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
     if (bs->file) {
         return bdrv_snapshot_delete(bs->file, snapshot_id, name, errp);
     }
-    error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
-              drv->format_name, bdrv_get_device_name(bs),
-              "internal snapshot deletion");
+    error_setg(errp, "Block format '%s' used by device '%s' "
+               "does not support internal snapshot deletion",
+               drv->format_name, bdrv_get_device_name(bs));
     return -ENOTSUP;
 }

@@ -329,9 +329,9 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
     if (drv->bdrv_snapshot_load_tmp) {
         return drv->bdrv_snapshot_load_tmp(bs, snapshot_id, name, errp);
     }
-    error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
-              drv->format_name, bdrv_get_device_name(bs),
-              "temporarily load internal snapshot");
+    error_setg(errp, "Block format '%s' used by device '%s' "
+               "does not support temporarily loading internal snapshots",
+               drv->format_name, bdrv_get_device_name(bs));
     return -ENOTSUP;
 }

block/vdi.c +3 -3

index 53bd02f..7642ef3 100644
@@ -502,9 +502,9 @@ 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");
+    error_setg(&s->migration_blocker, "The vdi format used by node '%s' "
+               "does not support live migration",
+               bdrv_get_device_or_node_name(bs));
     migrate_add_blocker(s->migration_blocker);
 
     qemu_co_mutex_init(&s->write_lock);

block/vhdx.c +3 -3

index bb3ed45..01970c2 100644
@@ -1002,9 +1002,9 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
     /* TODO: differencing files */
 
     /* 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");
+    error_setg(&s->migration_blocker, "The vhdx format used by node '%s' "
+               "does not support live migration",
+               bdrv_get_device_or_node_name(bs));
     migrate_add_blocker(s->migration_blocker);
 
     return 0;

block/vmdk.c +4 -4

index 8410a15..fd94b8f 100644
@@ -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

@@ -962,9 +962,9 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
     qemu_co_mutex_init(&s->lock);
 
     /* 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");
+    error_setg(&s->migration_blocker, "The vmdk format used by node '%s' "
+               "does not support live migration",
+               bdrv_get_device_or_node_name(bs));
     migrate_add_blocker(s->migration_blocker);
     g_free(buf);
     return 0;

block/vpc.c +3 -3

index 43e768e..37572ba 100644
@@ -318,9 +318,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
     qemu_co_mutex_init(&s->lock);
 
     /* 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");
+    error_setg(&s->migration_blocker, "The vpc format used by node '%s' "
+               "does not support live migration",
+               bdrv_get_device_or_node_name(bs));
     migrate_add_blocker(s->migration_blocker);
 
     return 0;

block/vvfat.c +4 -3

index 9be632f..e803589 100644
@@ -1180,9 +1180,10 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
 
     /* Disable migration when vvfat is used rw */
     if (s->qcow) {
-        error_set(&s->migration_blocker,
-                  QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
-                  "vvfat (rw)", bdrv_get_device_name(bs), "live migration");
+        error_setg(&s->migration_blocker,
+                   "The vvfat (rw) format used by node '%s' "
+                   "does not support live migration",
+                   bdrv_get_device_or_node_name(bs));
         migrate_add_blocker(s->migration_blocker);
     }

blockdev.c +5 -4

index fbb3a79..30dc9d2 100644
@@ -1248,13 +1248,14 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
     }
 
     if (bdrv_is_read_only(bs)) {
-        error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
+        error_setg(errp, "Device '%s' is read only", device);
         return;
     }
 
     if (!bdrv_can_snapshot(bs)) {
-        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
-                  bs->drv->format_name, device, "internal snapshot");
+        error_setg(errp, "Block format '%s' used by device '%s' "
+                   "does not support internal snapshots",
+                   bs->drv->format_name, device);
         return;
     }

@@ -2055,7 +2056,7 @@ void qmp_block_resize(bool has_device, const char *device,
         error_set(errp, QERR_UNSUPPORTED);
         break;
     case -EACCES:
-        error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
+        error_setg(errp, "Device '%s' is read only", device);
         break;
     case -EBUSY:
         error_set(errp, QERR_DEVICE_IN_USE, device);

include/qapi/qmp/qerror.h +0 -6

index 57a62d4..e567339 100644
@@ -37,9 +37,6 @@ void qerror_report_err(Error *err);
 #define QERR_BASE_NOT_FOUND \
     ERROR_CLASS_GENERIC_ERROR, "Base '%s' not found"
 
-#define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \
-    ERROR_CLASS_GENERIC_ERROR, "Block format '%s' used by device '%s' does not support feature '%s'"
-
 #define QERR_BLOCK_JOB_NOT_READY \
     ERROR_CLASS_GENERIC_ERROR, "The active block job for device '%s' cannot be completed"

@@ -58,9 +55,6 @@ void qerror_report_err(Error *err);
 #define QERR_DEVICE_IN_USE \
     ERROR_CLASS_GENERIC_ERROR, "Device '%s' is in use"
 
-#define QERR_DEVICE_IS_READ_ONLY \
-    ERROR_CLASS_GENERIC_ERROR, "Device '%s' is read only"
-
 #define QERR_DEVICE_NO_HOTPLUG \
     ERROR_CLASS_GENERIC_ERROR, "Device '%s' does not support hotplugging"

Signature · 1 line

2.1.4

Max Reitz

Reviewed-by on v3 1/3

2015-03-20 19:12 UTC #2 lore raw

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

In reply to Alberto Garcia

Quoted 9 lines · 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 <[email protected]>
---
  block.c               | 9 +++++++++
  block/quorum.c        | 5 +----
  include/block/block.h | 1 +
  3 files changed, 11 insertions(+), 4 deletions(-)

No changes from v2, so:

Reviewed-by Max Reitz <[email protected]>

(Feel free to put these R-b lines into the commit message under your S-o-b for patches that got a R-b and don't have any changes)

Max Reitz

Reviewed-by on v3 2/3

2015-03-20 19:24 UTC #4 lore raw

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

In reply to Alberto Garcia

Quoted 10 lines · 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. The messages are also updated to use the more generic term 'node' instead of 'device'.
Signed-off-by Alberto Garcia <[email protected]>

Quoted 14 lines

---
  block.c                   | 24 ++++++++++++------------
  block/qcow.c              |  8 ++++----
  block/qcow2.c             |  2 +-
  block/qed.c               |  2 +-
  block/snapshot.c          | 12 ++++++------
  block/vdi.c               |  6 +++---
  block/vhdx.c              |  6 +++---
  block/vmdk.c              |  8 ++++----
  block/vpc.c               |  6 +++---
  block/vvfat.c             |  7 ++++---
  blockdev.c                |  9 +++++----
  include/qapi/qmp/qerror.h |  6 ------
  12 files changed, 46 insertions(+), 50 deletions(-)

Reviewed-by Max Reitz <[email protected]>

Max Reitz

Reviewed-by on v3 3/3

2015-03-20 19:27 UTC #6 lore raw

Re: [Qemu-devel] [PATCH 3/3] block: add 'node-name' field to BLOCK_IMAGE_CORRUPTED

In reply to Alberto Garcia

Quoted 9 lines · Alberto Garcia

Since this event can occur in nodes that cannot have a device name associated, include also a field with the node name.
Signed-off-by Alberto Garcia <[email protected]>
---
  block/qcow2.c           |  8 ++++++--
  docs/qmp/qmp-events.txt | 16 +++++++++-------
  qapi/block-core.json    | 13 ++++++++-----
  3 files changed, 23 insertions(+), 14 deletions(-)

Reviewed-by Max Reitz <[email protected]>

Markus Armbruster

2015-03-23 09:21 UTC #7 lore raw

Re: [Qemu-devel] [PATCH 3/3] block: add 'node-name' field to BLOCK_IMAGE_CORRUPTED

In reply to Alberto Garcia

Quoted 42 lines · Alberto Garcia

Since this event can occur in nodes that cannot have a device name associated, include also a field with the node name.

Signed-off-by Alberto Garcia <[email protected]>

---
 block/qcow2.c           |  8 ++++++--
 docs/qmp/qmp-events.txt | 16 +++++++++-------
 qapi/block-core.json    | 13 ++++++++-----
 3 files changed, 23 insertions(+), 14 deletions(-)

block/qcow2.c +6 -2

index 168006b..e7c78f1 100644
@@ -2809,6 +2809,7 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
                              int64_t size, const char *message_format, ...)
 {
     BDRVQcowState *s = bs->opaque;
+    const char *node_name;
     char *message;
     va_list ap;

@@ -2832,8 +2833,11 @@ 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,
+    node_name = bdrv_get_node_name(bs);
+    qapi_event_send_block_image_corrupted(bdrv_get_device_name(bs),
+                                          *node_name != '\0', node_name,
+                                          message, offset >= 0, offset,
+                                          size >= 0, size,
                                           fatal, &error_abort);
     g_free(message);

docs/qmp/qmp-events.txt +0 -0

index d759d19..ed1d0a5 100644
@@ -35,17 +35,19 @@ Emitted when a disk image is being marked corrupt.

docs/qmp/qmp-events.txt Emitted when a disk image is being marked corrupt. · line 39

Quoted 10 lines

docs/qmp/qmp-events.txt +2 -6

 Data:
 
-- "device": Device name (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)
-- "size":   If the corruption resulted from an image access, this is the access
-            size (json-int)
+- "device":    Device name (json-string)
+- "node-name": Node name, if it's present (json-string)

By convention, we mark optional members like this:

- "node-name": Node name (json-string, optional)

Ignorant question: can "device" be ""? If yes, we should document what that means, possibly in a separate patch.

Quoted 46 lines

docs/qmp/qmp-events.txt +7 -1

+- "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)
+- "size":      If the corruption resulted from an image access, this
+               is the access size (json-int)
 
 Example:
 
 { "event": "BLOCK_IMAGE_CORRUPTED",
-    "data": { "device": "ide0-hd0",
+    "data": { "device": "ide0-hd0", "node-name": "node0",
         "msg": "Prevented active L1 table overwrite", "offset": 196608,
         "size": 65536 },
     "timestamp": { "seconds": 1378126126, "microseconds": 966463 } }

qapi/block-core.json +8 -5

index f525b04..2a40b73 100644
@@ -1752,6 +1752,8 @@
 #
 # @device: device name
 #
+# @node-name: #optional node name (Since: 2.4)
+#
 # @msg: informative message for human consumption, such as the kind of
 #       corruption being detected. It should not be parsed by machine as it is
 #       not guaranteed to be stable

@@ -1769,11 +1771,12 @@
 # Since: 1.7
 ##
 { 'event': 'BLOCK_IMAGE_CORRUPTED',
-  'data': { 'device' : 'str',
-            'msg'    : 'str',
-            '*offset': 'int',
-            '*size'  : 'int',
-            'fatal'  : 'bool' } }
+  'data': { 'device'     : 'str',
+            '*node-name' : 'str',
+            'msg'        : 'str',
+            '*offset'    : 'int',
+            '*size'      : 'int',
+            'fatal'      : 'bool' } }
 
 ##
 # @BLOCK_IO_ERROR

Alberto Garcia

author

2015-03-23 09:53 UTC #8 lore raw

Re: [Qemu-devel] [PATCH 3/3] block: add 'node-name' field to BLOCK_IMAGE_CORRUPTED

Quoted 6 lines · Markus Armbruster

Quoted 2 lines
+- "device":    Device name (json-string)
+- "node-name": Node name, if it's present (json-string)
By convention, we mark optional members like this:
- "node-name": Node name (json-string, optional)

You're right, thanks for pointing that out!

This can be fixed when committing the patch, otherwise I can resend the series again.

Quoted 2 lines

Ignorant question: can "device" be ""? If yes, we should document what that means, possibly in a separate patch.

Yes it can be "", the purpose of the patch is precisely to expand the event with a new field that can identify the node in such cases.

The only reason why it is not optional is to keep backward compatibility.

But it's true that it can be clarified in the documentation, I can send a follow-up patch as soon as this one is merged.

Berto

Markus Armbruster

2015-03-23 11:55 UTC #9 lore raw

Re: [Qemu-devel] [PATCH 3/3] block: add 'node-name' field to BLOCK_IMAGE_CORRUPTED

Quoted 1 line · Alberto Garcia

On Mon, Mar 23, 2015 at 10:20:31AM +0100, Markus Armbruster wrote:

[...]

Quoted 11 lines

Quoted 2 lines
Ignorant question: can "device" be ""? If yes, we should document what that means, possibly in a separate patch.
Yes it can be "", the purpose of the patch is precisely to expand the event with a new field that can identify the node in such cases.
The only reason why it is not optional is to keep backward compatibility.
But it's true that it can be clarified in the documentation, I can send a follow-up patch as soon as this one is merged.

Follow-up patch works for me.