* [PATCH v3 2/2] hw/block/block.c: improve confusing blk_check_size_and_read_all() error
2024-01-30 7:30 [PATCH v3 0/2] hw/block/block.c: improve confusing error Manos Pitsidianakis
2024-01-30 7:30 ` [PATCH v3 1/2] hw/core/qdev.c: add qdev_get_human_name() Manos Pitsidianakis
@ 2024-01-30 7:30 ` Manos Pitsidianakis
2024-01-30 21:52 ` [PATCH v3 0/2] hw/block/block.c: improve confusing error Stefan Hajnoczi
2 siblings, 0 replies; 4+ messages in thread
From: Manos Pitsidianakis @ 2024-01-30 7:30 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Stefan Hajnoczi, Philippe Mathieu-Daudé, John Snow,
Kevin Wolf, Hanna Reitz, Alistair Francis
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>
---
include/hw/block/block.h | 4 ++--
hw/block/block.c | 25 +++++++++++++++----------
hw/block/m25p80.c | 3 ++-
hw/block/pflash_cfi01.c | 4 ++--
hw/block/pflash_cfi02.c | 2 +-
5 files changed, 22 insertions(+), 16 deletions(-)
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 */
diff --git a/hw/block/block.c b/hw/block/block.c
index 9f52ee6e72..ec4a675490 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 '%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 '%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;
--
γαῖα πυρί μιχθήτω
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v3 0/2] hw/block/block.c: improve confusing error
2024-01-30 7:30 [PATCH v3 0/2] hw/block/block.c: improve confusing error Manos Pitsidianakis
2024-01-30 7:30 ` [PATCH v3 1/2] hw/core/qdev.c: add qdev_get_human_name() Manos Pitsidianakis
2024-01-30 7:30 ` [PATCH v3 2/2] hw/block/block.c: improve confusing blk_check_size_and_read_all() error Manos Pitsidianakis
@ 2024-01-30 21:52 ` Stefan Hajnoczi
2 siblings, 0 replies; 4+ messages in thread
From: Stefan Hajnoczi @ 2024-01-30 21:52 UTC (permalink / raw)
To: Manos Pitsidianakis; +Cc: qemu-devel, qemu-block, Philippe Mathieu-Daudé
[-- Attachment #1: Type: text/plain, Size: 3255 bytes --]
On Tue, Jan 30, 2024 at 09:30:30AM +0200, Manos Pitsidianakis wrote:
> 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 3:
> - Changed phrasing "%s device with id='%s'" to "%s device '%s'" since
> second parameter might be either device id or device path.
> (thanks Stefan Hajnoczi <stefanha@redhat.com>)
>
> 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
>
> include/hw/block/block.h | 4 ++--
> include/hw/qdev-core.h | 14 ++++++++++++++
> 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 ++++++++
> 7 files changed, 44 insertions(+), 16 deletions(-)
>
> Range-diff against v2:
> 1: 5fb5879708 ! 1: 8b566bfced hw/core/qdev.c: add qdev_get_human_name()
> @@ Commit message
> Add a simple method to return some kind of human readable identifier for
> use in error messages.
>
> + Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
>
> ## include/hw/qdev-core.h ##
> 2: 8e7eb17fbd ! 2: 7260eadff2 hw/block/block.c: improve confusing blk_check_size_and_read_all() error
> @@ hw/block/block.c: static int blk_pread_nonzeroes(BlockBackend *blk, hwaddr size,
> - "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
> ++ error_setg(errp, "%s device '%s' requires %" HWADDR_PRIu
> + " bytes, %s block backend provides %" PRIu64 " bytes",
> + object_get_typename(OBJECT(dev)), dev_id, size,
> + blk_name(blk), blk_len);
> @@ hw/block/block.c: bool blk_check_size_and_read_all(BlockBackend *blk, void *buf,
> - 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'",
> ++ " for %s device '%s'",
> + blk_name(blk), object_get_typename(OBJECT(dev)),
> + dev_id);
> return false;
>
> base-commit: 11be70677c70fdccd452a3233653949b79e97908
> --
> γαῖα πυρί μιχθήτω
>
Thanks, applied to my block tree:
https://gitlab.com/stefanha/qemu/commits/block
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread