On Tue, 10 Mar 2026 at 13:27, Markus Armbruster <[email protected]> wrote: > > Alessandro Ratti <[email protected]> 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: <[email protected]> >
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/[email protected]/T/#m89da9b4e30b7c84713ca4b6c323514c72897e649 > > [2]: > > https://lore.kernel.org/qemu-devel/[email protected]/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 <[email protected]> > > Signed-off-by: Alessandro Ratti <[email protected]> > > --- > > ... 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
