Re: [PATCH] qdev: Consolidate qdev_get_human_name() into qdev_get_printable_name()

[Markus Armbruster <armbru@redhat.com>]

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>

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);