Replace virtio_error() with a macro that automatically prepends the
calling function name to error messages. This provides better context
for debugging virtio issues by showing exactly which function
encountered the error.

Before: "Invalid queue size: 1024"
After:  "virtio_queue_set_num: Invalid queue size: 1024"

The implementation uses a macro to insert __func__ at compile time,
avoiding any runtime overhead while providing more specific error
context than a generic "virtio:" prefix.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/230
Buglink: https://bugs.launchpad.net/qemu/+bug/1919021

Signed-off-by: Alessandro Ratti <alessandro@0x65c.net>
---
 hw/virtio/virtio.c         | 2 +-
 include/hw/virtio/virtio.h | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 9a81ad912e..44528d7f2b 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3931,7 +3931,7 @@ void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name)
     vdev->bus_name = g_strdup(bus_name);
 }
 
-void G_GNUC_PRINTF(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
+void G_GNUC_PRINTF(2, 3) virtio_error_impl(VirtIODevice *vdev, const char *fmt, ...)
 {
     va_list ap;
 
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index c594764f23..961d021497 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -249,7 +249,9 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size);
 
 void virtio_cleanup(VirtIODevice *vdev);
 
-void virtio_error(VirtIODevice *vdev, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
+#define virtio_error(vdev, fmt, ...) \
+    virtio_error_impl(vdev, "%s: " fmt, __func__, ##__VA_ARGS__)
+void virtio_error_impl(VirtIODevice *vdev, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
 
 /* Set the child bus name. */
 void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
-- 
2.39.5

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

> Replace virtio_error() with a macro that automatically prepends the
> calling function name to error messages. This provides better context
> for debugging virtio issues by showing exactly which function
> encountered the error.
>
> Before: "Invalid queue size: 1024"
> After:  "virtio_queue_set_num: Invalid queue size: 1024"
>
> The implementation uses a macro to insert __func__ at compile time,
> avoiding any runtime overhead while providing more specific error
> context than a generic "virtio:" prefix.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/230
> Buglink: https://bugs.launchpad.net/qemu/+bug/1919021
>
> Signed-off-by: Alessandro Ratti <alessandro@0x65c.net>
> ---
>  hw/virtio/virtio.c         | 2 +-
>  include/hw/virtio/virtio.h | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 9a81ad912e..44528d7f2b 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3931,7 +3931,7 @@ void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name)
>      vdev->bus_name = g_strdup(bus_name);
>  }
>  
> -void G_GNUC_PRINTF(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
> +void G_GNUC_PRINTF(2, 3) virtio_error_impl(VirtIODevice *vdev, const char *fmt, ...)
>  {
>      va_list ap;
>  
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c594764f23..961d021497 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -249,7 +249,9 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size);
>  
>  void virtio_cleanup(VirtIODevice *vdev);
>  
> -void virtio_error(VirtIODevice *vdev, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
> +#define virtio_error(vdev, fmt, ...) \
> +    virtio_error_impl(vdev, "%s: " fmt, __func__, ##__VA_ARGS__)
> +void virtio_error_impl(VirtIODevice *vdev, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
>  
>  /* Set the child bus name. */
>  void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);

For completeness you could also fixup:

  virtio_error(vdev, "%s: %d reason unknown", __func__, pnd->reason);

for virtio-ballon. Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

> For completeness you could also fixup:
> 
>   virtio_error(vdev, "%s: %d reason unknown", __func__, pnd->reason);
> 
> for virtio-ballon. Otherwise:
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Thank you for your review.
I've missed that. For completeness, I've sent out a v2 that removes the
manual __func__ usage in virtio-balloon to avoid duplicate function names,
as you suggested.


Thank you for your time and consideration.

Best regards,
Alessandro Ratti

Replace virtio_error() with a macro that automatically prepends the
calling function name to error messages. This provides better context
for debugging virtio issues by showing exactly which function
encountered the error.

Before: "Invalid queue size: 1024"
After:  "virtio_queue_set_num: Invalid queue size: 1024"

The implementation uses a macro to insert __func__ at compile time,
avoiding any runtime overhead while providing more specific error
context than a generic "virtio:" prefix.

Also remove manual __func__ usage in virtio-balloon to avoid duplicate
function names in error messages.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/230
Buglink: https://bugs.launchpad.net/qemu/+bug/1919021

Signed-off-by: Alessandro Ratti <alessandro@0x65c.net>
---
 hw/virtio/virtio-balloon.c | 2 +-
 hw/virtio/virtio.c         | 2 +-
 include/hw/virtio/virtio.h | 4 +++-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index db787d00b3..e443f71c01 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -697,7 +697,7 @@ virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data,
     case PRECOPY_NOTIFY_COMPLETE:
         break;
     default:
-        virtio_error(vdev, "%s: %d reason unknown", __func__, pnd->reason);
+        virtio_error(vdev, "%d reason unknown", pnd->reason);
     }
 
     return 0;
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 9a81ad912e..44528d7f2b 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3931,7 +3931,7 @@ void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name)
     vdev->bus_name = g_strdup(bus_name);
 }
 
-void G_GNUC_PRINTF(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
+void G_GNUC_PRINTF(2, 3) virtio_error_impl(VirtIODevice *vdev, const char *fmt, ...)
 {
     va_list ap;
 
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index c594764f23..961d021497 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -249,7 +249,9 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size);
 
 void virtio_cleanup(VirtIODevice *vdev);
 
-void virtio_error(VirtIODevice *vdev, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
+#define virtio_error(vdev, fmt, ...) \
+    virtio_error_impl(vdev, "%s: " fmt, __func__, ##__VA_ARGS__)
+void virtio_error_impl(VirtIODevice *vdev, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
 
 /* Set the child bus name. */
 void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
-- 
2.39.5

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

> Replace virtio_error() with a macro that automatically prepends the
> calling function name to error messages. This provides better context
> for debugging virtio issues by showing exactly which function
> encountered the error.
>
> Before: "Invalid queue size: 1024"
> After:  "virtio_queue_set_num: Invalid queue size: 1024"
>
> The implementation uses a macro to insert __func__ at compile time,
> avoiding any runtime overhead while providing more specific error
> context than a generic "virtio:" prefix.

A need for function names and such in error messages suggests the error
messages are crap.

Consider the example above.  From users' point of view, the message
changes from gobbledygook to more verbose gobbledygook.  Was the error
the user's fault?  The guest's?  Something else's?  What can the user do
about it now?  If you need a developer to answer such questions, the
user interface is *dire*.

Clue: virtio_error() sets vdev->broken to true.  Did the device just
stop working?  If yes, shouldn't we tell the user?

Note that __func__ does not materially improve things even for developer
when the error message template string is unique.  Almost all are.

Fun example: "Region caches not initialized".  Three instances:

hw/virtio/virtio.c:        virtio_error(vdev, "Region caches not initialized");
hw/virtio/virtio.c:        virtio_error(vdev, "Region caches not initialized");
hw/virtio/virtio.c:            error_setg(errp, "Region caches not initialized");

Your patch adds __func__ in two out of three cases.  I'm not asking you
to add it to the third case, I'm only mentioning this to illustrate the
depth of the error reporting swamp around here.

I'll shut up now :)

> Also remove manual __func__ usage in virtio-balloon to avoid duplicate
> function names in error messages.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/230
> Buglink: https://bugs.launchpad.net/qemu/+bug/1919021
>
> Signed-off-by: Alessandro Ratti <alessandro@0x65c.net>

On Mon, Sep 22, 2025 at 12:37:57PM +0200, Markus Armbruster wrote:
> Alessandro Ratti <alessandro@0x65c.net> writes:
> 
> > Replace virtio_error() with a macro that automatically prepends the
> > calling function name to error messages. This provides better context
> > for debugging virtio issues by showing exactly which function
> > encountered the error.
> >
> > Before: "Invalid queue size: 1024"
> > After:  "virtio_queue_set_num: Invalid queue size: 1024"
> >
> > The implementation uses a macro to insert __func__ at compile time,
> > avoiding any runtime overhead while providing more specific error
> > context than a generic "virtio:" prefix.
> 
> A need for function names and such in error messages suggests the error
> messages are crap.

I pretty much agree. If we take that view forwards, then I think our
coding guidelines should explicitly state something like

 "Function names must never be included in error messages.

  The messages need to be sufficiently descriptive in their
  text, such that including function names is redundant"

> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/230

This is interesting as it shows a link to a previously proposed patch:

  https://patchwork.kernel.org/project/qemu-devel/patch/20220414112902.41390-1-codeguy.moteen@gmail.com/

this old patch just expanded the error messages to include 'Virtio '
in their text. I'm not going to claim this made new error messages
hugely user friendly, but I think that old patch approach was at
least conceptually better & preferrable to the function name
addition.

> > Buglink: https://bugs.launchpad.net/qemu/+bug/1919021
> >
> > Signed-off-by: Alessandro Ratti <alessandro@0x65c.net>

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 :|

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Sep 22, 2025 at 12:37:57PM +0200, Markus Armbruster wrote:
>> Alessandro Ratti <alessandro@0x65c.net> writes:
>> 
>> > Replace virtio_error() with a macro that automatically prepends the
>> > calling function name to error messages. This provides better context
>> > for debugging virtio issues by showing exactly which function
>> > encountered the error.
>> >
>> > Before: "Invalid queue size: 1024"
>> > After:  "virtio_queue_set_num: Invalid queue size: 1024"
>> >
>> > The implementation uses a macro to insert __func__ at compile time,
>> > avoiding any runtime overhead while providing more specific error
>> > context than a generic "virtio:" prefix.
>> 
>> A need for function names and such in error messages suggests the error
>> messages are crap.
>
> I pretty much agree. If we take that view forwards, then I think our
> coding guidelines should explicitly state something like
>
>  "Function names must never be included in error messages.
>
>   The messages need to be sufficiently descriptive in their
>   text, such that including function names is redundant"

Ahh I missed the fact this ends up as an error_report. I think having
function names in debug output is fine.

It does however miss important information like which VirtIO device is
actually failing, despite having vdev passed down to the function.

>
>> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/230
>
> This is interesting as it shows a link to a previously proposed patch:
>
>   https://patchwork.kernel.org/project/qemu-devel/patch/20220414112902.41390-1-codeguy.moteen@gmail.com/
>
> this old patch just expanded the error messages to include 'Virtio '
> in their text. I'm not going to claim this made new error messages
> hugely user friendly, but I think that old patch approach was at
> least conceptually better & preferrable to the function name
> addition.
>
>> > Buglink: https://bugs.launchpad.net/qemu/+bug/1919021
>> >
>> > Signed-off-by: Alessandro Ratti <alessandro@0x65c.net>
>
> With regards,
> Daniel

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Alex Bennée <alex.bennee@linaro.org> writes:

> Daniel P. Berrangé <berrange@redhat.com> writes:
>
>> On Mon, Sep 22, 2025 at 12:37:57PM +0200, Markus Armbruster wrote:
>>> Alessandro Ratti <alessandro@0x65c.net> writes:
>>> 
>>> > Replace virtio_error() with a macro that automatically prepends the
>>> > calling function name to error messages. This provides better context
>>> > for debugging virtio issues by showing exactly which function
>>> > encountered the error.
>>> >
>>> > Before: "Invalid queue size: 1024"
>>> > After:  "virtio_queue_set_num: Invalid queue size: 1024"
>>> >
>>> > The implementation uses a macro to insert __func__ at compile time,
>>> > avoiding any runtime overhead while providing more specific error
>>> > context than a generic "virtio:" prefix.
>>> 
>>> A need for function names and such in error messages suggests the error
>>> messages are crap.
>>
>> I pretty much agree. If we take that view forwards, then I think our
>> coding guidelines should explicitly state something like
>>
>>  "Function names must never be included in error messages.
>>
>>   The messages need to be sufficiently descriptive in their
>>   text, such that including function names is redundant"

I'm in favor.

> Ahh I missed the fact this ends up as an error_report. I think having
> function names in debug output is fine.

No argument!

> It does however miss important information like which VirtIO device is
> actually failing, despite having vdev passed down to the function.

Yes, which device failed should definitely be reported.

[...]

On Mon, 22 Sept 2025 at 14:29, Markus Armbruster <armbru@redhat.com> wrote:
>
> Alex Bennée <alex.bennee@linaro.org> writes:
>
> > Daniel P. Berrangé <berrange@redhat.com> writes:
> >
> >> On Mon, Sep 22, 2025 at 12:37:57PM +0200, Markus Armbruster wrote:
> >>> Alessandro Ratti <alessandro@0x65c.net> writes:
> >>>
> >>> > Replace virtio_error() with a macro that automatically prepends the
> >>> > calling function name to error messages. This provides better context
> >>> > for debugging virtio issues by showing exactly which function
> >>> > encountered the error.
> >>> >
> >>> > Before: "Invalid queue size: 1024"
> >>> > After:  "virtio_queue_set_num: Invalid queue size: 1024"
> >>> >
> >>> > The implementation uses a macro to insert __func__ at compile time,
> >>> > avoiding any runtime overhead while providing more specific error
> >>> > context than a generic "virtio:" prefix.
> >>>
> >>> A need for function names and such in error messages suggests the error
> >>> messages are crap.
> >>
> >> I pretty much agree. If we take that view forwards, then I think our
> >> coding guidelines should explicitly state something like
> >>
> >>  "Function names must never be included in error messages.
> >>
> >>   The messages need to be sufficiently descriptive in their
> >>   text, such that including function names is redundant"
>
> I'm in favor.
>
> > Ahh I missed the fact this ends up as an error_report. I think having
> > function names in debug output is fine.
>
> No argument!
>
> > It does however miss important information like which VirtIO device is
> > actually failing, despite having vdev passed down to the function.
>
> Yes, which device failed should definitely be reported.
>
> [...]

Hi Markus, Alex, Daniel,

Thanks again for the thoughtful feedback and for helping me see the bigger
picture. I now fully agree that adding function names to error messages (via
__func__) doesn't really address the core issue, and I appreciate the
push to rethink how error reporting can better serve both users and developers.

I've taken a first stab at improving one of the messages in
virtio_init_region_cache(), following your suggestions.

Here's the updated call:

---8<--- Example diff --8<---
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -240,6 +240,7 @@ void virtio_init_region_cache(VirtIODevice *vdev, int n)
     VirtQueue *vq = &vdev->vq[n];
     VRingMemoryRegionCaches *old = vq->vring.caches;
     VRingMemoryRegionCaches *new = NULL;
+    DeviceState *dev = DEVICE(vdev);
     hwaddr addr, size;
     int64_t len;
     bool packed;
@@ -264,7 +265,10 @@ void virtio_init_region_cache(VirtIODevice *vdev, int n)
     len = address_space_cache_init(&new->used, vdev->dma_as,
                                    vq->vring.used, size, true);
     if (len < size) {
-        virtio_error(vdev, "Cannot map used");
+        virtio_error(vdev,
+                    "Failed to map used ring for device %s - "
+                    "possible guest misconfiguration or insufficient memory",
+                    qdev_get_dev_path(dev));
         goto err_used;
     }

With this change, the error output now reads:

    qemu-system-x86_64: Failed to map used ring for device
0000:00:04.0 - possible guest misconfiguration or insufficient memory

This feels like a clear improvement — it gives context (what failed),
identifies the device, and hints at likely causes.

If this is more in line with what you'd expect, I'd be happy to submit a new
patch focused solely on improving a few key virtio error messages in this
direction, starting with the worst offenders in virtio_init_region_cache().

Thanks again for your time and guidance — I'm learning a lot from this process.

Best regards,
Alessandro

Alessandro Ratti <alessandro.ratti@gmail.com> writes:

> On Mon, 22 Sept 2025 at 14:29, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Alex Bennée <alex.bennee@linaro.org> writes:
>>
>> > Daniel P. Berrangé <berrange@redhat.com> writes:
>> >
>> >> On Mon, Sep 22, 2025 at 12:37:57PM +0200, Markus Armbruster wrote:
>> >>> Alessandro Ratti <alessandro@0x65c.net> writes:
>> >>>
>> >>> > Replace virtio_error() with a macro that automatically prepends the
>> >>> > calling function name to error messages. This provides better context
>> >>> > for debugging virtio issues by showing exactly which function
>> >>> > encountered the error.
>> >>> >
>> >>> > Before: "Invalid queue size: 1024"
>> >>> > After:  "virtio_queue_set_num: Invalid queue size: 1024"
>> >>> >
>> >>> > The implementation uses a macro to insert __func__ at compile time,
>> >>> > avoiding any runtime overhead while providing more specific error
>> >>> > context than a generic "virtio:" prefix.
>> >>>
>> >>> A need for function names and such in error messages suggests the error
>> >>> messages are crap.
>> >>
>> >> I pretty much agree. If we take that view forwards, then I think our
>> >> coding guidelines should explicitly state something like
>> >>
>> >>  "Function names must never be included in error messages.
>> >>
>> >>   The messages need to be sufficiently descriptive in their
>> >>   text, such that including function names is redundant"
>>
>> I'm in favor.
>>
>> > Ahh I missed the fact this ends up as an error_report. I think having
>> > function names in debug output is fine.
>>
>> No argument!
>>
>> > It does however miss important information like which VirtIO device is
>> > actually failing, despite having vdev passed down to the function.
>>
>> Yes, which device failed should definitely be reported.
>>
>> [...]
>
> Hi Markus, Alex, Daniel,
>
> Thanks again for the thoughtful feedback and for helping me see the bigger
> picture. I now fully agree that adding function names to error messages (via
> __func__) doesn't really address the core issue, and I appreciate the
> push to rethink how error reporting can better serve both users and developers.
>
> I've taken a first stab at improving one of the messages in
> virtio_init_region_cache(), following your suggestions.
>
> Here's the updated call:
>
> ---8<--- Example diff --8<---
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -240,6 +240,7 @@ void virtio_init_region_cache(VirtIODevice *vdev, int n)
>      VirtQueue *vq = &vdev->vq[n];
>      VRingMemoryRegionCaches *old = vq->vring.caches;
>      VRingMemoryRegionCaches *new = NULL;
> +    DeviceState *dev = DEVICE(vdev);
>      hwaddr addr, size;
>      int64_t len;
>      bool packed;
> @@ -264,7 +265,10 @@ void virtio_init_region_cache(VirtIODevice *vdev, int n)
>      len = address_space_cache_init(&new->used, vdev->dma_as,
>                                     vq->vring.used, size, true);
>      if (len < size) {
> -        virtio_error(vdev, "Cannot map used");
> +        virtio_error(vdev,
> +                    "Failed to map used ring for device %s - "
> +                    "possible guest misconfiguration or insufficient memory",
> +                    qdev_get_dev_path(dev));
>          goto err_used;
>      }
>
> With this change, the error output now reads:
>
>     qemu-system-x86_64: Failed to map used ring for device
> 0000:00:04.0 - possible guest misconfiguration or insufficient memory
>
> This feels like a clear improvement — it gives context (what failed),
> identifies the device, and hints at likely causes.

It's *much* better!

Developers will appreciate "Failed to map used ring for device".  By
itself it would still be gobbledygook for users, but together with the
"possible guest misconfiguration or insufficient memory" clue it's fine.

Perhaps we can still improve on "device 0000:00:04.0".  The device's ID
is a good way to identify it to the user, because it's chosen by the
user, and unique (among devices).  Sadly, devices without ID exist.  We
fall back to canonical QOM path in places.  Have a look at
qdev_get_human_name() to see whether it works here.

Should we spell out that the device is now broken (whatever vdev->broken
means)?

> If this is more in line with what you'd expect, I'd be happy to submit a new
> patch focused solely on improving a few key virtio error messages in this
> direction, starting with the worst offenders in virtio_init_region_cache().
>
> Thanks again for your time and guidance — I'm learning a lot from this process.

You're welcome!  And thank you for accepting my little rant as
constructive criticism :)

On Mon, 22 Sept 2025 at 16:23, Markus Armbruster <armbru@redhat.com> wrote:
>
> Alessandro Ratti <alessandro.ratti@gmail.com> writes:
>
> > On Mon, 22 Sept 2025 at 14:29, Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Alex Bennée <alex.bennee@linaro.org> writes:
> >>
> >> > Daniel P. Berrangé <berrange@redhat.com> writes:
> >> >
> >> >> On Mon, Sep 22, 2025 at 12:37:57PM +0200, Markus Armbruster wrote:
> >> >>> Alessandro Ratti <alessandro@0x65c.net> writes:
> >> >>>
> >> >>> > Replace virtio_error() with a macro that automatically prepends the
> >> >>> > calling function name to error messages. This provides better context
> >> >>> > for debugging virtio issues by showing exactly which function
> >> >>> > encountered the error.
> >> >>> >
> >> >>> > Before: "Invalid queue size: 1024"
> >> >>> > After:  "virtio_queue_set_num: Invalid queue size: 1024"
> >> >>> >
> >> >>> > The implementation uses a macro to insert __func__ at compile time,
> >> >>> > avoiding any runtime overhead while providing more specific error
> >> >>> > context than a generic "virtio:" prefix.
> >> >>>
> >> >>> A need for function names and such in error messages suggests the error
> >> >>> messages are crap.
> >> >>
> >> >> I pretty much agree. If we take that view forwards, then I think our
> >> >> coding guidelines should explicitly state something like
> >> >>
> >> >>  "Function names must never be included in error messages.
> >> >>
> >> >>   The messages need to be sufficiently descriptive in their
> >> >>   text, such that including function names is redundant"
> >>
> >> I'm in favor.
> >>
> >> > Ahh I missed the fact this ends up as an error_report. I think having
> >> > function names in debug output is fine.
> >>
> >> No argument!
> >>
> >> > It does however miss important information like which VirtIO device is
> >> > actually failing, despite having vdev passed down to the function.
> >>
> >> Yes, which device failed should definitely be reported.
> >>
> >> [...]
> >
> > Hi Markus, Alex, Daniel,
> >
> > Thanks again for the thoughtful feedback and for helping me see the bigger
> > picture. I now fully agree that adding function names to error messages (via
> > __func__) doesn't really address the core issue, and I appreciate the
> > push to rethink how error reporting can better serve both users and developers.
> >
> > I've taken a first stab at improving one of the messages in
> > virtio_init_region_cache(), following your suggestions.
> >
> > Here's the updated call:
> >
> > ---8<--- Example diff --8<---
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -240,6 +240,7 @@ void virtio_init_region_cache(VirtIODevice *vdev, int n)
> >      VirtQueue *vq = &vdev->vq[n];
> >      VRingMemoryRegionCaches *old = vq->vring.caches;
> >      VRingMemoryRegionCaches *new = NULL;
> > +    DeviceState *dev = DEVICE(vdev);
> >      hwaddr addr, size;
> >      int64_t len;
> >      bool packed;
> > @@ -264,7 +265,10 @@ void virtio_init_region_cache(VirtIODevice *vdev, int n)
> >      len = address_space_cache_init(&new->used, vdev->dma_as,
> >                                     vq->vring.used, size, true);
> >      if (len < size) {
> > -        virtio_error(vdev, "Cannot map used");
> > +        virtio_error(vdev,
> > +                    "Failed to map used ring for device %s - "
> > +                    "possible guest misconfiguration or insufficient memory",
> > +                    qdev_get_dev_path(dev));
> >          goto err_used;
> >      }
> >
> > With this change, the error output now reads:
> >
> >     qemu-system-x86_64: Failed to map used ring for device
> > 0000:00:04.0 - possible guest misconfiguration or insufficient memory
> >
> > This feels like a clear improvement — it gives context (what failed),
> > identifies the device, and hints at likely causes.
>
> It's *much* better!
>
> Developers will appreciate "Failed to map used ring for device".  By
> itself it would still be gobbledygook for users, but together with the
> "possible guest misconfiguration or insufficient memory" clue it's fine.
>
> Perhaps we can still improve on "device 0000:00:04.0".  The device's ID
> is a good way to identify it to the user, because it's chosen by the
> user, and unique (among devices).  Sadly, devices without ID exist.  We
> fall back to canonical QOM path in places.  Have a look at
> qdev_get_human_name() to see whether it works here.

I experimented with qdev_get_human_name(), but it usually returns paths like:

  /machine/peripheral-anon/device[0]/virtio-backend

…which seems less user-friendly than the PCI address provided by
qdev_get_dev_path().
For now, I'm sticking to using the device ID when set (e.g. via -device…,id=foo)
and falling back to qdev_get_dev_path() otherwise — which provides predictable
output for both PCI and non-PCI devices.

> Should we spell out that the device is now broken (whatever vdev->broken
> means)?

That's a good point — I'll leave that for a possible follow-up so we can keep
this patch focused on improving error clarity and device identification first.

I'll submit a new patch shortly based on this discussion, with the
debug scaffolding removed. I'll include a link to this thread in the cover
letter for continuity.

> > If this is more in line with what you'd expect, I'd be happy to submit a new
> > patch focused solely on improving a few key virtio error messages in this
> > direction, starting with the worst offenders in virtio_init_region_cache().
> >
> > Thanks again for your time and guidance — I'm learning a lot from this process.
>
> You're welcome!  And thank you for accepting my little rant as
> constructive criticism :)

:)
Thanks again for your time and guidance — it's really helping me learn the ropes
here.

Best regards,
Alessandro

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

> On Mon, 22 Sept 2025 at 16:23, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Alessandro Ratti <alessandro.ratti@gmail.com> writes:
>>
>> > Hi Markus, Alex, Daniel,
>> >
>> > Thanks again for the thoughtful feedback and for helping me see the bigger
>> > picture. I now fully agree that adding function names to error messages (via
>> > __func__) doesn't really address the core issue, and I appreciate the
>> > push to rethink how error reporting can better serve both users and developers.
>> >
>> > I've taken a first stab at improving one of the messages in
>> > virtio_init_region_cache(), following your suggestions.
>> >
>> > Here's the updated call:

[...]

>> > With this change, the error output now reads:
>> >
>> >     qemu-system-x86_64: Failed to map used ring for device
>> > 0000:00:04.0 - possible guest misconfiguration or insufficient memory
>> >
>> > This feels like a clear improvement — it gives context (what failed),
>> > identifies the device, and hints at likely causes.
>>
>> It's *much* better!
>>
>> Developers will appreciate "Failed to map used ring for device".  By
>> itself it would still be gobbledygook for users, but together with the
>> "possible guest misconfiguration or insufficient memory" clue it's fine.
>>
>> Perhaps we can still improve on "device 0000:00:04.0".  The device's ID
>> is a good way to identify it to the user, because it's chosen by the
>> user, and unique (among devices).  Sadly, devices without ID exist.  We
>> fall back to canonical QOM path in places.  Have a look at
>> qdev_get_human_name() to see whether it works here.
>
> I experimented with qdev_get_human_name(), but it usually returns paths like:
>
>   /machine/peripheral-anon/device[0]/virtio-backend
>
> …which seems less user-friendly than the PCI address provided by
> qdev_get_dev_path().
> For now, I'm sticking to using the device ID when set (e.g. via -device…,id=foo)
> and falling back to qdev_get_dev_path() otherwise — which provides predictable
> output for both PCI and non-PCI devices.

Note that qdev_get_dev_path() may return null.  You need another
fallback.

For what it's worth, "qdev ID or QOM path" is how users specify devices
in QMP.

[...]

> > For completeness you could also fixup:
> >
> >   virtio_error(vdev, "%s: %d reason unknown", __func__, pnd->reason);
> >
> > for virtio-ballon. Otherwise:
> >
> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 
> Thank you for your review.
> I've missed that. For completeness, I've sent out a v2 that removes the
> manual __func__ usage in virtio-balloon to avoid duplicate function names,
> as you suggested.

Resending v2 with additional CCs to virtio maintainers and Michael
Tokarev (trivial-patches), as no feedback was received after initial
posting.

Thank you for your time and consideration.

Best regards,
Alessandro Ratti

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

>> > For completeness you could also fixup:
>> >
>> >   virtio_error(vdev, "%s: %d reason unknown", __func__, pnd->reason);
>> >
>> > for virtio-ballon. Otherwise:
>> >
>> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> 
>> Thank you for your review.
>> I've missed that. For completeness, I've sent out a v2 that removes the
>> manual __func__ usage in virtio-balloon to avoid duplicate function names,
>> as you suggested.
>
> Resending v2 with additional CCs to virtio maintainers and Michael
> Tokarev (trivial-patches), as no feedback was received after initial
> posting.

You should have added my Reviewed-by to the commit. You'll find tools
like b4 can help you with this:

  https://qemu.readthedocs.io/en/v10.0.3/devel/submitting-a-patch.html#proper-use-of-reviewed-by-tags-can-aid-review

>
> Thank you for your time and consideration.
>
> Best regards,
> Alessandro Ratti

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Replace virtio_error() with a macro that automatically prepends the
calling function name to error messages. This provides better context
for debugging virtio issues by showing exactly which function
encountered the error.

Before: "Invalid queue size: 1024"
After:  "virtio_queue_set_num: Invalid queue size: 1024"

The implementation uses a macro to insert __func__ at compile time,
avoiding any runtime overhead while providing more specific error
context than a generic "virtio:" prefix.

Also remove manual __func__ usage in virtio-balloon to avoid duplicate
function names in error messages.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/230
Buglink: https://bugs.launchpad.net/qemu/+bug/1919021

Signed-off-by: Alessandro Ratti <alessandro@0x65c.net>
---
 hw/virtio/virtio-balloon.c | 2 +-
 hw/virtio/virtio.c         | 2 +-
 include/hw/virtio/virtio.h | 4 +++-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index db787d00b3..e443f71c01 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -697,7 +697,7 @@ virtio_balloon_free_page_hint_notify(NotifierWithReturn *n, void *data,
     case PRECOPY_NOTIFY_COMPLETE:
         break;
     default:
-        virtio_error(vdev, "%s: %d reason unknown", __func__, pnd->reason);
+        virtio_error(vdev, "%d reason unknown", pnd->reason);
     }
 
     return 0;
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 9a81ad912e..44528d7f2b 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -3931,7 +3931,7 @@ void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name)
     vdev->bus_name = g_strdup(bus_name);
 }
 
-void G_GNUC_PRINTF(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, ...)
+void G_GNUC_PRINTF(2, 3) virtio_error_impl(VirtIODevice *vdev, const char *fmt, ...)
 {
     va_list ap;
 
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index c594764f23..961d021497 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -249,7 +249,9 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size);
 
 void virtio_cleanup(VirtIODevice *vdev);
 
-void virtio_error(VirtIODevice *vdev, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
+#define virtio_error(vdev, fmt, ...) \
+    virtio_error_impl(vdev, "%s: " fmt, __func__, ##__VA_ARGS__)
+void virtio_error_impl(VirtIODevice *vdev, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
 
 /* Set the child bus name. */
 void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name);
-- 
2.39.5