Re: [PATCH v3] virtio: improve virtqueue mapping error messages

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 24 Sept 2025 at 10:37, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>
>> On Wed, Sep 24, 2025 at 11:14:04AM +0200, Alessandro Ratti wrote:
>> > Improve error reporting when virtqueue ring mapping fails by including a
>> > device identifier in the error message.
>> >
>> > Introduce a helper qdev_get_printable_name() in qdev-core, which returns
>> > either:
>> >
>> >  - the device ID, if explicitly provided (e.g. -device ...,id=foo)
>> >  - the QOM path from qdev_get_dev_path(dev) otherwise
>> >  - "<unknown device>" as a fallback when no identifier is present
>> >
>> > This makes it easier to identify which device triggered the error in
>> > multi-device setups or when debugging complex guest configurations.
>> >
>> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/230
>> > Buglink: https://bugs.launchpad.net/qemu/+bug/1919021
>> >
>> > Suggested-by: Markus Armbruster <armbru@redhat.com>
>> > Signed-off-by: Alessandro Ratti <alessandro@0x65c.net>
>> > ---
>> >  hw/core/qdev.c         | 29 +++++++++++++++++++++++++++++
>> >  hw/virtio/virtio.c     | 15 ++++++++++++---
>> >  include/hw/qdev-core.h |  1 +
>> >  3 files changed, 42 insertions(+), 3 deletions(-)
>>
>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
> Hi; I just found this commit (now e209d4d7a31b9) in the course
> of finding a memory leak in it. I'm about to send patches to fix
> that, but in the meantime:
>
> We now have two different functions for "give me a nice string
> that I can use in an error message about this device":
>
>  * qdev_get_human_name(), used only in hw/block/block.c
>  * qdev_get_printable_name(), used only in hw/virtio/virtio.c
>
> Can we please have *one* function for this purpose?

Sensible request.

> I see from the thread that the criticism of qdev_get_human_name()
> is that "it often returns opaque paths like
>
>    /machine/peripheral-anon/device[0]/virtio-backend
>
> which are less informative in user-facing logs compared to PCI IDs or
> user-specified names".
>
> So could we instead make block.c use qdev_get_printable_name(), and
> remove qdev_get_human_name() ? It also is using the result only
> to construct an error string for the user.

No, not without significant improvements to qdev_get_printable_name().

Let's have a closer look.


First qdev_get_human_name():

    char *qdev_get_human_name(DeviceState *dev)
    {
        g_assert(dev != NULL);

        return dev->id ?
               g_strdup(dev->id) : object_get_canonical_path(OBJECT(dev));
    }

This returns the device's ID if it has one, else its canonical QOM path.

Intepreting the value is easy enough: if it starts with '/', it's a
canonical QOM path, else a qdev ID.

Devices plugged by the user have an ID if the user specified one.  If
they don't have one, the canonical path will be
/machine/peripheral-anon/device[N], where N is a non-negative integer.
Gibberish, but easy enough to avoid: specify an ID.  Management
applications should do that always.

Onboard devices do not have an ID.  The return value could be something
relatively reasonable like /machine/soc/cpu, or gibberish like
/machine/unattached/device[N].


Next qdev_get_printable_name():

    const char *qdev_get_printable_name(DeviceState *vdev)

Nitpick: @vdev is an unusual identifier.  We normally use @dev around
here.

    {
        /*
         * 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 vdev->id;
        }

Same as qdev_get_human_name() so far.

        /*
         * Fall back to the canonical QOM device path (eg. ID for PCI
         * devices).
         * 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 cibus_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.

The comment is nonsense: this has nothing to do with "the canonical QOM
device path".

        /*
         * Final fallback: if all else fails, return a placeholder string.
         * This ensures the error message always contains a valid string.
         */
        return "<unknown device>";

This fallback is is unnecessarily bad.  The canonical QOM path always
exists and would be far better.

    }

Intepreting the value can be difficult: we have some twenty
.get_dev_path() methods, and each of them can format however it wants.
You need to guess the format to make sense of the value.


Neither function is ideal.  But in their current state, I *strongly*
prefer qdev_get_human_name(), because I hate having to decipher a QOM
path less than having to guess what kind of device this might be, then
figure out what format its .get_dev_path() uses.

If someone posts a patch to fix the shortcomings of
qdev_get_printable_name() I outlined above, we can talk.