Re: [PATCH v3] virtio: improve virtqueue mapping error messages
Peter Maydell writes: > On Tue, 10 Mar 2026 at 11:03, Markus Armbruster wrote: >> 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. > > How do you feel about the proposed unification of the two > functions done in this patch? > > https://lore.kernel.org/qemu-devel/[email protected]/ Thanks for the pointer, I'll reply there.
Re: [PATCH v3] virtio: improve virtqueue mapping error messages
On Tue, 10 Mar 2026 at 11:03, Markus Armbruster wrote: > 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. How do you feel about the proposed unification of the two functions done in this patch? https://lore.kernel.org/qemu-devel/[email protected]/ thanks -- PMM
Re: [PATCH v3] virtio: improve virtqueue mapping error messages
Peter Maydell writes:
> On Wed, 24 Sept 2025 at 10:37, Daniel P. Berrangé 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
>> > - "" 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
>> > Signed-off-by: Alessandro Ratti
>> > ---
>> > 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é
>
> 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 "";
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 t
Re: [PATCH v3] virtio: improve virtqueue mapping error messages
On Sat, 7 Mar 2026 at 14:39, Peter Maydell wrote: > > On Wed, 24 Sept 2025 at 10:37, Daniel P. Berrangé 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 > > > - "" 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 > > > Signed-off-by: Alessandro Ratti > > > --- > > > 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é > > 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? > > 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. > Hi Peter, Thank you for catching the memory leak and for the feedback on the API design. You're right that having two similar functions is confusing and unnecessarily duplicates functionality. I agree that consolidating to a single function makes sense. I can send a follow-up patch to: 1. Replace the usage of qdev_get_human_name() in hw/block/block.c with qdev_get_printable_name() 2. Remove qdev_get_human_name() entirely 3. Fix the memory leak in qdev_get_printable_name() (unless you already have a patch ready to send) Alternatively, if you prefer to handle the consolidation as part of your memory leak fix series, please feel free to do so - just let me know. Thank you for your time and for improving the code. Best regards, Alessandro
Re: [PATCH v3] virtio: improve virtqueue mapping error messages
On Wed, 24 Sept 2025 at 10:37, Daniel P. Berrangé 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 > > - "" 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 > > Signed-off-by: Alessandro Ratti > > --- > > 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é 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? 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. thanks -- PMM
Re: [PATCH v3] virtio: improve virtqueue mapping error messages
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 > - "" 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 > Signed-off-by: Alessandro Ratti > --- > 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é With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
