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]> 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. 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 <[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) > @@ -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);
