Re: [PATCH v2 1/2] hw/qdev: Clarify fallback order in qdev_get_printable_name()

[Markus Armbruster <armbru@redhat.com>]

Alessandro Ratti <alessandro@0x65c.net> writes:

> Replace the uninformative "<unknown device>" final fallback with the
> canonical QOM path (e.g. /machine/peripheral-anon/device[0]).
>
> Also clean up comments to accurately describe qdev_get_dev_path()
> behavior, drop an unnecessary comment on the dev->id check, rename
> the @vdev parameter to @dev for consistency with surrounding code,
> and add g_assert(dev) to match qdev_get_human_name()'s precondition.
>
> Update the doc comment in qdev.h to reflect the new fallback chain.
>
> Signed-off-by: Alessandro Ratti <alessandro@0x65c.net>
> ---
>  hw/core/qdev.c         | 29 ++++++++++++-----------------
>  include/hw/core/qdev.h | 11 +++++------
>  2 files changed, 17 insertions(+), 23 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index e48616b2c6..f9fb7966dd 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -411,33 +411,28 @@ char *qdev_get_dev_path(DeviceState *dev)
>      return NULL;
>  }
>  
> -const char *qdev_get_printable_name(DeviceState *vdev)
> +const char *qdev_get_printable_name(DeviceState *dev)
>  {
> -    /*
> -     * 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.
> -     */
> -    if (vdev->id) {
> -        return g_strdup(vdev->id);
> +    g_assert(dev != NULL);

This replaces crash on dev->id below by an assertion failure.  I
wouldn't bother.

> +
> +    if (dev->id) {
> +        return g_strdup(dev->id);
>      }
> +
>      /*
> -     * Fall back to the canonical QOM device path (eg. ID for PCI
> -     * devices).
> -     * This ensures the device is still uniquely and meaningfully
> -     * identified.
> +     * Fall back to a bus-specific device path, if the bus
> +     * provides one (e.g. PCI address "0000:00:04.0").
>       */
> -    const char *path = qdev_get_dev_path(vdev);
> +    const char *path = qdev_get_dev_path(dev);
>      if (path) {
>          return path;
>      }
>  
>      /*
> -     * 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
> +     * (e.g. /machine/peripheral-anon/device[0]).
>       */

Is the comment useful?

> -    return g_strdup("<unknown device>");
> +    return object_get_canonical_path(OBJECT(dev));
>  }
>  
>  void qdev_add_unplug_blocker(DeviceState *dev, Error *reason)
> diff --git a/include/hw/core/qdev.h b/include/hw/core/qdev.h
> index f99a8979cc..2da7327144 100644
> --- a/include/hw/core/qdev.h
> +++ b/include/hw/core/qdev.h
> @@ -1087,18 +1087,17 @@ char *qdev_get_dev_path(DeviceState *dev);
>  
>  /**
>   * qdev_get_printable_name: Return human readable name for device
> - * @dev: Device to get name of
> + * @dev: Device to get name of. Must not be NULL.
>   *
>   * Returns: A newly allocated string containing some human
>   * readable name for the device, suitable for printing in
> - * user-facing error messages. The function will never return NULL,
> - * so the name can be used without further checking or fallbacks.
> + * user-facing error messages.
>   *
>   * If the device has an explicitly set ID (e.g. by the user on the
>   * command line via "-device thisdev,id=myid") this is preferred.
> - * Otherwise we try the canonical QOM device path (which will be
> - * the PCI ID for PCI devices, for example). If all else fails
> - * we will return the placeholder "<unknown device">.
> + * Otherwise we try the bus-specific device path (which will be
> + * the PCI address for PCI devices, for example). If all else fails
> + * we return the canonical QOM path.

I prefer imperative mood for describing function behavior:

      Return the device's ID if it has one.  Else, return the path of a
      device on its bus if it has one.  Else return its canonical QOM
      path.

Bonus: obviously not the longwinded, flowery prose LLMs barf out[*].

>   */
>  const char *qdev_get_printable_name(DeviceState *dev);


[*] Would not be welcome in QEMU at this time; see section "Use of
AI-generated content" in docs/devel/code-provenance.rst.