Peter Maydell <[email protected]> writes:
> On Wed, 24 Sept 2025 at 10:37, Daniel P. Berrangé <[email protected]> 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 <[email protected]>
>> > Signed-off-by: Alessandro Ratti <[email protected]>
>> > ---
>> > 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é <[email protected]>
>
> 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.
