hw/block/block.c | 5 ++--- hw/core/qdev.c | 19 ++++++++----------- include/hw/core/qdev.h | 13 ------------- 3 files changed, 10 insertions(+), 27 deletions(-)
Hello,
This patch consolidates qdev_get_human_name() and qdev_get_printable_name()
into a single function, as suggested by Peter Maydell here [1]
This patch is based on Peter's recent series and should be applied after [2]:
- "hw/qdev: Document qdev_get_dev_path()"
- "hw: Make qdev_get_printable_name() consistently return freeable string"
Thank you for your time and consideration.
Best regards
Alessandro
[1]: https://lore.kernel.org/qemu-devel/aNLIHOwcGB47qbUY@redhat.com/T/#m89da9b4e30b7c84713ca4b6c323514c72897e649
[2]: https://lore.kernel.org/qemu-devel/20260307155046.3940197-1-peter.maydell@linaro.org/T/#m962127cb58192e0b2095039cb2fb79145f2a7388
---
Remove qdev_get_human_name() and use qdev_get_printable_name() for all
device identification in error messages.
Both functions now behave similarly, with qdev_get_printable_name()
preferring bus-specific paths (e.g., PCI addresses) when available
before falling back to QOM canonical paths. This provides better
context in error messages while maintaining the same level of detail.
Error messages will now show device identifiers in this priority:
1. User-specified device IDs (e.g., -device virtio-blk,id=foo)
2. Bus-specific identifiers (e.g., PCI addresses like 0000:00:04.0)
3. QOM canonical paths as a last resort
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Alessandro Ratti <alessandro@0x65c.net>
---
hw/block/block.c | 5 ++---
hw/core/qdev.c | 19 ++++++++-----------
include/hw/core/qdev.h | 13 -------------
3 files changed, 10 insertions(+), 27 deletions(-)
diff --git a/hw/block/block.c b/hw/block/block.c
index f187fa025d..84e5298e2f 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -65,7 +65,6 @@ bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
{
int64_t blk_len;
int ret;
- g_autofree char *dev_id = NULL;
if (cpr_is_incoming()) {
return true;
@@ -78,7 +77,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
return false;
}
if (blk_len != size) {
- dev_id = qdev_get_human_name(dev);
+ g_autofree const char *dev_id = qdev_get_printable_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,
@@ -95,7 +94,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
assert(size <= BDRV_REQUEST_MAX_BYTES);
ret = blk_pread_nonzeroes(blk, size, buf);
if (ret < 0) {
- dev_id = qdev_get_human_name(dev);
+ g_autofree const char *dev_id = qdev_get_printable_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)),
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index e48616b2c6..9ee98a0c39 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -434,10 +434,15 @@ const char *qdev_get_printable_name(DeviceState *vdev)
}
/*
- * Final fallback: if all else fails, return a placeholder string.
- * This ensures the error message always contains a valid string.
+ * Final fallback: return the canonical QOM path.
+ * While verbose (e.g., /machine/peripheral-anon/device[0]), this
+ * provides accurate device identification when neither a user-specified
+ * ID nor a bus-specific path is available. Only falls back to
+ * <unknown device> in the extremely rare case where even the QOM
+ * path is unavailable.
*/
- return g_strdup("<unknown device>");
+ char *qom_path = object_get_canonical_path(OBJECT(vdev));
+ return qom_path ? qom_path : g_strdup("<unknown device>");
}
void qdev_add_unplug_blocker(DeviceState *dev, Error *reason)
@@ -867,14 +872,6 @@ Object *machine_get_container(const char *name)
return container;
}
-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/core/qdev.h b/include/hw/core/qdev.h
index f99a8979cc..09dafd3d59 100644
--- a/include/hw/core/qdev.h
+++ b/include/hw/core/qdev.h
@@ -1045,19 +1045,6 @@ void qdev_create_fake_machine(void);
*/
Object *machine_get_container(const char *name);
-/**
- * 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);
--
2.53.0Alessandro Ratti <alessandro@0x65c.net> writes:
> Hello,
>
> This patch consolidates qdev_get_human_name() and qdev_get_printable_name()
> into a single function, as suggested by Peter Maydell here [1]
>
> This patch is based on Peter's recent series and should be applied after [2]:
> - "hw/qdev: Document qdev_get_dev_path()"
> - "hw: Make qdev_get_printable_name() consistently return freeable string"
Use
Based-on: <20260307155046.3940197-1-peter.maydell@linaro.org>
so that tools like patchew can apply patches without human intervention.
> Thank you for your time and consideration.
>
> Best regards
> Alessandro
>
> [1]: https://lore.kernel.org/qemu-devel/aNLIHOwcGB47qbUY@redhat.com/T/#m89da9b4e30b7c84713ca4b6c323514c72897e649
> [2]: https://lore.kernel.org/qemu-devel/20260307155046.3940197-1-peter.maydell@linaro.org/T/#m962127cb58192e0b2095039cb2fb79145f2a7388
>
> ---
git-am uses part above the --- line as commit message. That's clearly
not what you intended.
To fix it, put your introduction ...
> Remove qdev_get_human_name() and use qdev_get_printable_name() for all
> device identification in error messages.
I think qdev_get_human_name() is the nicer name of the two. Matter of
taste.
> Both functions now behave similarly, with qdev_get_printable_name()
Both functions? You remove one!
> preferring bus-specific paths (e.g., PCI addresses) when available
> before falling back to QOM canonical paths. This provides better
> context in error messages while maintaining the same level of detail.
>
> Error messages will now show device identifiers in this priority:
> 1. User-specified device IDs (e.g., -device virtio-blk,id=foo)
> 2. Bus-specific identifiers (e.g., PCI addresses like 0000:00:04.0)
> 3. QOM canonical paths as a last resort
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Alessandro Ratti <alessandro@0x65c.net>
> ---
... here instead. Or simply use a cover letter.
> hw/block/block.c | 5 ++---
> hw/core/qdev.c | 19 ++++++++-----------
> include/hw/core/qdev.h | 13 -------------
> 3 files changed, 10 insertions(+), 27 deletions(-)
>
> diff --git a/hw/block/block.c b/hw/block/block.c
> index f187fa025d..84e5298e2f 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -65,7 +65,6 @@ bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
> {
> int64_t blk_len;
> int ret;
> - g_autofree char *dev_id = NULL;
>
> if (cpr_is_incoming()) {
> return true;
> @@ -78,7 +77,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
> return false;
> }
> if (blk_len != size) {
> - dev_id = qdev_get_human_name(dev);
> + g_autofree const char *dev_id = qdev_get_printable_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,
I believe this plugs a memory leak. Separate patch, please, with
Fixes: 954b33daee83 (hw/block/block.c: improve confusing blk_check_size_and_read_all() error)
> @@ -95,7 +94,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
> assert(size <= BDRV_REQUEST_MAX_BYTES);
> ret = blk_pread_nonzeroes(blk, size, buf);
> if (ret < 0) {
> - dev_id = qdev_get_human_name(dev);
> + g_autofree const char *dev_id = qdev_get_printable_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)),
Likewise.
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index e48616b2c6..9ee98a0c39 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -434,10 +434,15 @@ const char *qdev_get_printable_name(DeviceState *vdev)
const char *qdev_get_printable_name(DeviceState *vdev)
Nitpick: @vdev is an unusual identifier. Please use @dev like the code
nearby.
{
/*
* Return device ID if explicity set
* (e.g. -device virtio-blk-pci,id=foo)
* This allows users to correlate errors with their custom device
* names.
*/
I doubt this comment is worth its keep.
if (vdev->id) {
return g_strdup(vdev->id);
}
/*
* Fall back to the canonical QOM device path (eg. ID for PCI
* devices).
This comment is mostly nonsense: there's no such thing as a "canonical
QOM device path". There's a canonical QOM path (the path from root to
object in the QOM composition tree), but qdev_get_dev_path() doesn't
return it.
* This ensures the device is still uniquely and meaningfully
* identified.
*/
const char *path = qdev_get_dev_path(vdev);
if (path) {
return path;
This returns a "name" in a bus-specific format if the device is plugged
into a bus that provides the get_dev_path() method. Many buses do.
For instance, the PCI bus's method is pcibus_get_dev_path(). It appears
to return a path of the form
Domain:00:Slot.Function:Slot.Function....:Slot.Function.
which is commonly just a PCI address.
Intepreting the value can be difficult: we have some twenty
.get_dev_path() methods, and each of them can format however it wants.
I need to guess the format to make sense of the value.
Could we do something like "<bus> device <dev-path>"? E.g.
PCI device 0000:00:00.0
> }
>
> /*
> - * Final fallback: if all else fails, return a placeholder string.
> - * This ensures the error message always contains a valid string.
> + * Final fallback: return the canonical QOM path.
> + * While verbose (e.g., /machine/peripheral-anon/device[0]), this
> + * provides accurate device identification when neither a user-specified
> + * ID nor a bus-specific path is available. Only falls back to
> + * <unknown device> in the extremely rare case where even the QOM
> + * path is unavailable.
This case isn't "extremely rare", it should never happen.
object_get_canonical_path(vdev) can only return null when @dev qom_path
can only be null when @vdev is not part of the QOM composition tree.
> */
> - return g_strdup("<unknown device>");
> + char *qom_path = object_get_canonical_path(OBJECT(vdev));
> + return qom_path ? qom_path : g_strdup("<unknown device>");
qom_path should never be null, and I wouldn't bother with a fallback.
This is not a demand. If you want a fallback, consider something like
g_strdup_printf("bad device %p").
This patch does two things: improve qdev_get_printable_name(), and
replace qdev_get_human_name(). Please split it.
> }
>
> void qdev_add_unplug_blocker(DeviceState *dev, Error *reason)
> @@ -867,14 +872,6 @@ Object *machine_get_container(const char *name)
> return container;
> }
>
> -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/core/qdev.h b/include/hw/core/qdev.h
> index f99a8979cc..09dafd3d59 100644
> --- a/include/hw/core/qdev.h
> +++ b/include/hw/core/qdev.h
> @@ -1045,19 +1045,6 @@ void qdev_create_fake_machine(void);
> */
> Object *machine_get_container(const char *name);
>
> -/**
> - * 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);
Please add a similarly nice contract to qdev_get_printable_name().
>
> /* FIXME: make this a link<> */
> bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
On Tue, 10 Mar 2026 at 13:27, Markus Armbruster <armbru@redhat.com> wrote:
>
> Alessandro Ratti <alessandro@0x65c.net> writes:
>
> > Hello,
> >
> > This patch consolidates qdev_get_human_name() and qdev_get_printable_name()
> > into a single function, as suggested by Peter Maydell here [1]
> >
> > This patch is based on Peter's recent series and should be applied after [2]:
> > - "hw/qdev: Document qdev_get_dev_path()"
> > - "hw: Make qdev_get_printable_name() consistently return freeable string"
>
> Use
>
> Based-on: <20260307155046.3940197-1-peter.maydell@linaro.org>
>
Will do.
> so that tools like patchew can apply patches without human intervention.
>
> > Thank you for your time and consideration.
> >
> > Best regards
> > Alessandro
> >
> > [1]: https://lore.kernel.org/qemu-devel/aNLIHOwcGB47qbUY@redhat.com/T/#m89da9b4e30b7c84713ca4b6c323514c72897e649
> > [2]: https://lore.kernel.org/qemu-devel/20260307155046.3940197-1-peter.maydell@linaro.org/T/#m962127cb58192e0b2095039cb2fb79145f2a7388
> >
> > ---
>
> git-am uses part above the --- line as commit message. That's clearly
> not what you intended.
Right, lesson learned. I'll move the introduction below the --- line in the
future.
>
> To fix it, put your introduction ...
>
>
> > Remove qdev_get_human_name() and use qdev_get_printable_name() for all
> > device identification in error messages.
>
> I think qdev_get_human_name() is the nicer name of the two. Matter of
> taste.
Fair enough, but since Peter's series already establishes
qdev_get_printable_name(), I kept that one.
>
> > Both functions now behave similarly, with qdev_get_printable_name()
>
> Both functions? You remove one!
Poor wording on my part, sorry. Will fix.
>
> > preferring bus-specific paths (e.g., PCI addresses) when available
> > before falling back to QOM canonical paths. This provides better
> > context in error messages while maintaining the same level of detail.
> >
> > Error messages will now show device identifiers in this priority:
> > 1. User-specified device IDs (e.g., -device virtio-blk,id=foo)
> > 2. Bus-specific identifiers (e.g., PCI addresses like 0000:00:04.0)
> > 3. QOM canonical paths as a last resort
> >
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Alessandro Ratti <alessandro@0x65c.net>
> > ---
>
> ... here instead. Or simply use a cover letter.
>
> > hw/block/block.c | 5 ++---
> > hw/core/qdev.c | 19 ++++++++-----------
> > include/hw/core/qdev.h | 13 -------------
> > 3 files changed, 10 insertions(+), 27 deletions(-)
> >
> > diff --git a/hw/block/block.c b/hw/block/block.c
> > index f187fa025d..84e5298e2f 100644
> > --- a/hw/block/block.c
> > +++ b/hw/block/block.c
> > @@ -65,7 +65,6 @@ bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
> > {
> > int64_t blk_len;
> > int ret;
> > - g_autofree char *dev_id = NULL;
> >
> > if (cpr_is_incoming()) {
> > return true;
> > @@ -78,7 +77,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
> > return false;
> > }
> > if (blk_len != size) {
> > - dev_id = qdev_get_human_name(dev);
> > + g_autofree const char *dev_id = qdev_get_printable_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,
>
> I believe this plugs a memory leak. Separate patch, please, with
>
> Fixes: 954b33daee83 (hw/block/block.c: improve confusing blk_check_size_and_read_all() error)
>
As Peter noted, the original dev_id is already g_autofree, so there's
no leak here - just narrowing the variable scope.
> > @@ -95,7 +94,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
> > assert(size <= BDRV_REQUEST_MAX_BYTES);
> > ret = blk_pread_nonzeroes(blk, size, buf);
> > if (ret < 0) {
> > - dev_id = qdev_get_human_name(dev);
> > + g_autofree const char *dev_id = qdev_get_printable_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)),
>
> Likewise.
Ditto.
>
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index e48616b2c6..9ee98a0c39 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -434,10 +434,15 @@ const char *qdev_get_printable_name(DeviceState *vdev)
> const char *qdev_get_printable_name(DeviceState *vdev)
>
> Nitpick: @vdev is an unusual identifier. Please use @dev like the code
> nearby.
Ack.
>
> {
> /*
> * Return device ID if explicity set
> * (e.g. -device virtio-blk-pci,id=foo)
> * This allows users to correlate errors with their custom device
> * names.
> */
>
> I doubt this comment is worth its keep.
Agreed, I'll drop it.
>
> if (vdev->id) {
> return g_strdup(vdev->id);
> }
> /*
> * Fall back to the canonical QOM device path (eg. ID for PCI
> * devices).
>
> This comment is mostly nonsense: there's no such thing as a "canonical
> QOM device path". There's a canonical QOM path (the path from root to
> object in the QOM composition tree), but qdev_get_dev_path() doesn't
> return it.
>
> * This ensures the device is still uniquely and meaningfully
> * identified.
> */
> const char *path = qdev_get_dev_path(vdev);
> if (path) {
> return path;
>
> This returns a "name" in a bus-specific format if the device is plugged
> into a bus that provides the get_dev_path() method. Many buses do.
>
You're right, I'll fix the comments to describe what qdev_get_dev_path()
returns based on your explanation.
> For instance, the PCI bus's method is pcibus_get_dev_path(). It appears
> to return a path of the form
>
> Domain:00:Slot.Function:Slot.Function....:Slot.Function.
>
> which is commonly just a PCI address.
>
> Intepreting the value can be difficult: we have some twenty
> .get_dev_path() methods, and each of them can format however it wants.
> I need to guess the format to make sense of the value.
>
> Could we do something like "<bus> device <dev-path>"? E.g.
>
> PCI device 0000:00:00.0
>
That's a nice idea. I think it deserves its own series though, since it
would involve changes to the bus get_dev_path() methods or to how
qdev_get_printable_name() formats the result.
I'll follow up on that separately.
> > }
> >
> > /*
> > - * Final fallback: if all else fails, return a placeholder string.
> > - * This ensures the error message always contains a valid string.
> > + * Final fallback: return the canonical QOM path.
> > + * While verbose (e.g., /machine/peripheral-anon/device[0]), this
> > + * provides accurate device identification when neither a user-specified
> > + * ID nor a bus-specific path is available. Only falls back to
> > + * <unknown device> in the extremely rare case where even the QOM
> > + * path is unavailable.
>
> This case isn't "extremely rare", it should never happen.
Understood.
> object_get_canonical_path(vdev) can only return null when @dev qom_path
> can only be null when @vdev is not part of the QOM composition tree.
>
> > */
> > - return g_strdup("<unknown device>");
> > + char *qom_path = object_get_canonical_path(OBJECT(vdev));
> > + return qom_path ? qom_path : g_strdup("<unknown device>");
>
> qom_path should never be null, and I wouldn't bother with a fallback.
Ack, I'll drop the fallback.
> This is not a demand. If you want a fallback, consider something like
>
> g_strdup_printf("bad device %p").
>
> This patch does two things: improve qdev_get_printable_name(), and
> replace qdev_get_human_name(). Please split it.
>
Ack, I'll split it into two patches in v2.
Thanks for the thorough review, I'll send a v2 addressing all of the above.
Best regards,
Alessandro
On Tue, 10 Mar 2026 at 12:27, Markus Armbruster <armbru@redhat.com> wrote:
> Or simply use a cover letter.
Patchew doesn't correctly handle cover-letter + single patch,
so my recommendation is "cover letter if and only if there
is more than one patch".
> > hw/block/block.c | 5 ++---
> > hw/core/qdev.c | 19 ++++++++-----------
> > include/hw/core/qdev.h | 13 -------------
> > 3 files changed, 10 insertions(+), 27 deletions(-)
> >
> > diff --git a/hw/block/block.c b/hw/block/block.c
> > index f187fa025d..84e5298e2f 100644
> > --- a/hw/block/block.c
> > +++ b/hw/block/block.c
> > @@ -65,7 +65,6 @@ bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
> > {
> > int64_t blk_len;
> > int ret;
> > - g_autofree char *dev_id = NULL;
> >
> > if (cpr_is_incoming()) {
> > return true;
> > @@ -78,7 +77,7 @@ bool blk_check_size_and_read_all(BlockBackend *blk, DeviceState *dev,
> > return false;
> > }
> > if (blk_len != size) {
> > - dev_id = qdev_get_human_name(dev);
> > + g_autofree const char *dev_id = qdev_get_printable_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,
>
> I believe this plugs a memory leak.
It doesn't -- the previous declaration of dev_id above also
is marked g_autofree. This patch is just moving the declarations
into a narrower scope.
> > -/**
> > - * 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);
>
> Please add a similarly nice contract to qdev_get_printable_name().
Mostly that is in
https://patchew.org/QEMU/20260307155046.3940197-1-peter.maydell@linaro.org/20260307155046.3940197-3-peter.maydell@linaro.org/
thanks
-- PMM
© 2016 - 2026 Red Hat, Inc.