[PATCH v2 0/2] hw/block/block.c: improve confusing error

[PATCH v2 0/2] hw/block/block.c: improve confusing error

[Manos Pitsidianakis]

In cases where a device tries to read more bytes than the block device
contains with the blk_check_size_and_read_all() function, the error is
vague: "device requires X bytes, block backend provides Y bytes".

This patch changes the errors of this function to include the block
backend name, the device id and device type name where appropriate.

Version 2:
- Assert dev is not NULL on qdev_get_human_name
    (thanks Phil Mathieu-Daudé <philmd@linaro.org>)

Manos Pitsidianakis (2):
  hw/core/qdev.c: add qdev_get_human_name()
  hw/block/block.c: improve confusing blk_check_size_and_read_all()
    error

 hw/block/block.c         | 25 +++++++++++++++----------
 hw/block/m25p80.c        |  3 ++-
 hw/block/pflash_cfi01.c  |  4 ++--
 hw/block/pflash_cfi02.c  |  2 +-
 hw/core/qdev.c           |  8 ++++++++
 include/hw/block/block.h |  4 ++--
 include/hw/qdev-core.h   | 14 ++++++++++++++
 7 files changed, 44 insertions(+), 16 deletions(-)

Range-diff against v1:
1:  15b15d6d4f ! 1:  5fb5879708 hw/core/qdev.c: add qdev_get_human_name()
    @@ hw/core/qdev.c: Object *qdev_get_machine(void)
      
     +char *qdev_get_human_name(DeviceState *dev)
     +{
    -+    if (!dev) {
    -+        return g_strdup("");
    -+    }
    ++    g_assert(dev != NULL);
     +
     +    return dev->id ?
     +           g_strdup(dev->id) : object_get_canonical_path(OBJECT(dev));
    @@ include/hw/qdev-core.h: const char *qdev_fw_name(DeviceState *dev);
      
     +/**
     + * qdev_get_human_name() - Return a human-readable name for a device
    -+ * @dev: The device
    ++ * @dev: The device. Must be a valid and non-NULL pointer.
     + *
     + * .. note::
     + *    This function is intended for user friendly error messages.
     + *
     + * Returns: A newly allocated string containing the device id if not null,
    -+ * else the object canonical path if not null. If @dev is NULL, it returns an
    -+ * allocated empty string.
    ++ * else the object canonical path.
     + *
     + * Use g_free() to free it.
     + */
2:  e3701762ed ! 2:  8e7eb17fbd hw/block/block.c: improve confusing blk_check_size_and_read_all() error
    @@ Commit message
         This patch changes the errors of this function to include the block
         backend name, the device id and device type name where appropriate.
     
    +    Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
         Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
     
      ## hw/block/block.c ##

base-commit: 09be34717190c1620f0c6e5c8765b8da354aeb4b
-- 
γαῖα πυρί μιχθήτω

[PATCH v2 1/2] hw/core/qdev.c: add qdev_get_human_name()

[Manos Pitsidianakis]

Add a simple method to return some kind of human readable identifier for
use in error messages.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/core/qdev.c         |  8 ++++++++
 include/hw/qdev-core.h | 14 ++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 43d863b0c5..c68d0f7c51 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -879,6 +879,14 @@ Object *qdev_get_machine(void)
     return dev;
 }
 
+char *qdev_get_human_name(DeviceState *dev)
+{
+    g_assert(dev != NULL);
+
+    return dev->id ?
+           g_strdup(dev->id) : object_get_canonical_path(OBJECT(dev));
+}
+
 static MachineInitPhase machine_phase;
 
 bool phase_check(MachineInitPhase phase)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 151d968238..66338f479f 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -993,6 +993,20 @@ const char *qdev_fw_name(DeviceState *dev);
 void qdev_assert_realized_properly(void);
 Object *qdev_get_machine(void);
 
+/**
+ * qdev_get_human_name() - Return a human-readable name for a device
+ * @dev: The device. Must be a valid and non-NULL pointer.
+ *
+ * .. note::
+ *    This function is intended for user friendly error messages.
+ *
+ * Returns: A newly allocated string containing the device id if not null,
+ * else the object canonical path.
+ *
+ * Use g_free() to free it.
+ */
+char *qdev_get_human_name(DeviceState *dev);
+
 /* FIXME: make this a link<> */
 bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
 
-- 
γαῖα πυρί μιχθήτω

Re: [PATCH v2 1/2] hw/core/qdev.c: add qdev_get_human_name()

[Stefan Hajnoczi]

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

On Tue, Jan 23, 2024 at 05:35:30PM +0200, Manos Pitsidianakis wrote:
> Add a simple method to return some kind of human readable identifier for
> use in error messages.
> 
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>  hw/core/qdev.c         |  8 ++++++++
>  include/hw/qdev-core.h | 14 ++++++++++++++
>  2 files changed, 22 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 2/2] hw/block/block.c: improve confusing blk_check_size_and_read_all() error

[Manos Pitsidianakis]

In cases where a device tries to read more bytes than the block device
contains, the error is vague: "device requires X bytes, block backend
provides Y bytes".

This patch changes the errors of this function to include the block
backend name, the device id and device type name where appropriate.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 hw/block/block.c         | 25 +++++++++++++++----------
 hw/block/m25p80.c        |  3 ++-
 hw/block/pflash_cfi01.c  |  4 ++--
 hw/block/pflash_cfi02.c  |  2 +-
 include/hw/block/block.h |  4 ++--
 5 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index 9f52ee6e72..624389d62d 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -54,29 +54,30 @@ static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr size, void *buf)
  * BDRV_REQUEST_MAX_BYTES.
  * On success, return true.
  * On failure, store an error through @errp and return false.
- * Note that the error messages do not identify the block backend.
- * TODO Since callers don't either, this can result in confusing
- * errors.
+ *
  * This function not intended for actual block devices, which read on
  * demand.  It's for things like memory devices that (ab)use a block
  * backend to provide persistence.
  */
-bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
-                                 Error **errp)
+bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
+                                 void *buf, hwaddr size, Error **errp)
 {
     int64_t blk_len;
     int ret;
+    g_autofree char *dev_id = NULL;
 
     blk_len = blk_getlength(blk);
     if (blk_len < 0) {
         error_setg_errno(errp, -blk_len,
-                         "can't get size of block backend");
+                         "can't get size of %s block backend", blk_name(blk));
         return false;
     }
     if (blk_len != size) {
-        error_setg(errp, "device requires %" HWADDR_PRIu " bytes, "
-                   "block backend provides %" PRIu64 " bytes",
-                   size, blk_len);
+        dev_id = qdev_get_human_name(dev);
+        error_setg(errp, "%s device with id='%s' requires %" HWADDR_PRIu
+                   " bytes, %s block backend provides %" PRIu64 " bytes",
+                   object_get_typename(OBJECT(dev)), dev_id, size,
+                   blk_name(blk), blk_len);
         return false;
     }
 
@@ -89,7 +90,11 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
     assert(size <= BDRV_REQUEST_MAX_BYTES);
     ret = blk_pread_nonzeroes(blk, size, buf);
     if (ret < 0) {
-        error_setg_errno(errp, -ret, "can't read block backend");
+        dev_id = qdev_get_human_name(dev);
+        error_setg_errno(errp, -ret, "can't read %s block backend"
+                         "for %s device with id='%s'",
+                         blk_name(blk), object_get_typename(OBJECT(dev)),
+                         dev_id);
         return false;
     }
     return true;
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 26ce895628..0a12030a3a 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -1617,7 +1617,8 @@ static void m25p80_realize(SSIPeripheral *ss, Error **errp)
         trace_m25p80_binding(s);
         s->storage = blk_blockalign(s->blk, s->size);
 
-        if (!blk_check_size_and_read_all(s->blk, s->storage, s->size, errp)) {
+        if (!blk_check_size_and_read_all(s->blk, DEVICE(s),
+                                         s->storage, s->size, errp)) {
             return;
         }
     } else {
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index f956f8bcf7..1bda8424b9 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -848,8 +848,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     }
 
     if (pfl->blk) {
-        if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
-                                         errp)) {
+        if (!blk_check_size_and_read_all(pfl->blk, dev, pfl->storage,
+                                         total_len, errp)) {
             vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
             return;
         }
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 6fa56f14c0..2314142373 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -902,7 +902,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
     }
 
     if (pfl->blk) {
-        if (!blk_check_size_and_read_all(pfl->blk, pfl->storage,
+        if (!blk_check_size_and_read_all(pfl->blk, dev, pfl->storage,
                                          pfl->chip_len, errp)) {
             vmstate_unregister_ram(&pfl->orig_mem, DEVICE(pfl));
             return;
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 15fff66435..de3946a5f1 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -88,8 +88,8 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
 
 /* Backend access helpers */
 
-bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
-                                 Error **errp);
+bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
+                                 void *buf, hwaddr size, Error **errp);
 
 /* Configuration helpers */
 
-- 
γαῖα πυρί μιχθήτω

Re: [PATCH v2 2/2] hw/block/block.c: improve confusing blk_check_size_and_read_all() error

[Stefan Hajnoczi]

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

On Tue, Jan 23, 2024 at 05:35:31PM +0200, Manos Pitsidianakis wrote:
>      if (blk_len != size) {
> -        error_setg(errp, "device requires %" HWADDR_PRIu " bytes, "
> -                   "block backend provides %" PRIu64 " bytes",
> -                   size, blk_len);
> +        dev_id = qdev_get_human_name(dev);
> +        error_setg(errp, "%s device with id='%s' requires %" HWADDR_PRIu

Since qdev_get_human_name() falls back to returning the path instead of
the id, this error message could be confusing. Perhaps avoid saying what
dev_id is and let the user interpret it:

  %s device '%s'

> +                   " bytes, %s block backend provides %" PRIu64 " bytes",
> +                   object_get_typename(OBJECT(dev)), dev_id, size,
> +                   blk_name(blk), blk_len);
>          return false;
>      }
>  
> @@ -89,7 +90,11 @@ bool blk_check_size_and_read_all(BlockBackend *blk, void *buf, hwaddr size,
>      assert(size <= BDRV_REQUEST_MAX_BYTES);
>      ret = blk_pread_nonzeroes(blk, size, buf);
>      if (ret < 0) {
> -        error_setg_errno(errp, -ret, "can't read block backend");
> +        dev_id = qdev_get_human_name(dev);
> +        error_setg_errno(errp, -ret, "can't read %s block backend"
> +                         "for %s device with id='%s'",

Same here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]