[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH 2/3] block: use bdrv_get_device_or_node_name() in error messages
Date: Fri, 20 Mar 2015 09:22:04 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0

On 2015-03-20 at 06:19, 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. The
messages are also updated to use the more generic term 'node' instead
of 'device'.

Signed-off-by: Alberto Garcia <address@hidden>
  block.c                   | 21 +++++++++++----------
  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 ++-
  blockdev.c                |  4 ++--
  include/qapi/qmp/qerror.h |  8 ++++----
  11 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index 98b90b4..8247f0a 100644
--- a/block.c
+++ b/block.c
@@ -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) {
-                   "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));

Actually, the change of "device" to "node" here is independent of the use of bdrv_get_device_or_node_name(). But it's correct anyway.

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_set(errp, QERR_NODE_IS_READ_ONLY,
+                  bdrv_get_device_or_node_name(reopen_state->bs));

I think Markus would be happier with just removing the macro.

(Make it error_setg(errp, "Node '%s' is read only", bdrv_get_device_or_node_name(reopen_state->bs)) and remove the QERR_DEVICE_IS_READ_ONLY macro from qerror.h)


diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 57a62d4..46cad14 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -38,7 +38,7 @@ void qerror_report_err(Error *err);
      ERROR_CLASS_GENERIC_ERROR, "Base '%s' not found"
-    ERROR_CLASS_GENERIC_ERROR, "Block format '%s' used by device '%s' does not 
support feature '%s'"
+    ERROR_CLASS_GENERIC_ERROR, "Block format '%s' used by node '%s' does not 
support feature '%s'"

Preexisting, but first: This line has over 80 characters.

Second: This seems like a good opportunity to drop this macro and replace its occurrences by error_setg(errp, "Block format...") and the like.

      ERROR_CLASS_GENERIC_ERROR, "The active block job for device '%s' cannot be 
@@ -58,9 +58,6 @@ void qerror_report_err(Error *err);
  #define QERR_DEVICE_IN_USE \
      ERROR_CLASS_GENERIC_ERROR, "Device '%s' is in use"
-    ERROR_CLASS_GENERIC_ERROR, "Device '%s' is read only"
      ERROR_CLASS_GENERIC_ERROR, "Device '%s' does not support hotplugging"
@@ -103,6 +100,9 @@ void qerror_report_err(Error *err);
      ERROR_CLASS_GENERIC_ERROR, "Parameter '%s' is missing"
+    ERROR_CLASS_GENERIC_ERROR, "Node '%s' is read only"
      ERROR_CLASS_GENERIC_ERROR, "Insufficient permission to perform this 

As said above, the same goes for this macro.

I'm not so ardent about this, so if you want to keep the macros, it's fine with me; but the overly long line need to be fixed (only the one you're touching, not necessarily the rest, because I guess they'll be dropped in the future anyway).



reply via email to

[Prev in Thread] Current Thread [Next in Thread]