Re: [PATCH] xen/passthrough: add missing error-report include

[Daniel P. Berrangé <berrange@xxxxxxxxxx>]

On Fri, Jul 18, 2025 at 07:59:50AM +0200, Markus Armbruster wrote:
> Markus Armbruster <armbru@xxxxxxxxxx> writes:
> 
> > Adam Williamson <awilliam@xxxxxxxxxx> writes:
> >
> >> In cfcacba an `error_report` was added to this file, but the
> >> corresponding include of `qemu/error-report.h` was missed. This
> >> only becomes apparent when building against Xen 4.20+.
> >>
> >> Signed-off-by: Adam Williamson <awilliam@xxxxxxxxxx>
> >> ---
> >>  hw/xen/xen_pt.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> >> index 9d16644d82..006b5b55f2 100644
> >> --- a/hw/xen/xen_pt.c
> >> +++ b/hw/xen/xen_pt.c
> >> @@ -54,6 +54,7 @@
> >>  
> >>  #include "qemu/osdep.h"
> >>  #include "qapi/error.h"
> >> +#include "qemu/error-report.h"
> >>  #include <sys/ioctl.h>
> >>  
> >>  #include "hw/pci/pci.h"
> >
> > Uh, error-report.h is included without this for me.  To see, build with
> > -H:
> >
> > . /work/armbru/qemu/hw/xen/xen_pt.h
> > .. /work/armbru/qemu/include/hw/xen/xen_native.h
> > ... /work/armbru/qemu/hw/xen/trace.h
> > .... ./trace/trace-hw_xen.h
> > ..... /work/armbru/qemu/include/qemu/error-report.h
> 
> Just remembered: the generated trace header includes error-report.h only
> when trace's log backend is enabled.

Hmm, that's rather an unfortunate trap-door :-( Given that 'log' is enabled
by default when building from git we'll never see missing error-report.h
problems in daily work.

Looking at the log backend it appears that originally it would
unconditionally include the timestamp when calling qemu_log, but
then changed that to opt-in with

  commit 418ed14268f797a5142b60cd557cd598eb548c66
  Author: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
  Date:   Mon Jan 25 11:35:07 2021 +0000

    trace: make the 'log' backend timestamp configurable

requiring -msg timestamp=on, which was a pre-existing flag that already
did a similar toggle for the 'error_report' function. The goal makes
sense, but it introduced the error-report.h trap door

When I see that I also question why the 'log' backend should be a
special case user of qemu_log() ?  Why shouldn't we emit timestamps
for all usage of qemu_log() ?

If we changed the qemu_log impl to honour the timestamp toggle, then
all users of qemu_log benefit. We then eliminate error-report.h usage
in trace.h headers, and also cut the code size for trace points
significantly


static inline void _nocheck__trace_object_dynamic_cast_assert(const char * 
type, const char * target, const char * file, int line, const char * func)
{
    if (trace_event_get_state(TRACE_OBJECT_DYNAMIC_CAST_ASSERT) && 
qemu_loglevel_mask(LOG_TRACE)) {
        if (message_with_timestamp) {
            struct timeval _now;
            gettimeofday(&_now, NULL);
            qemu_log("%d@%zu.%06zu:object_dynamic_cast_assert " "%s->%s 
(%s:%d:%s)" "\n",
                     qemu_get_thread_id(),
                     (size_t)_now.tv_sec, (size_t)_now.tv_usec
                     , type, target, file, line, func);
        } else {
            qemu_log("object_dynamic_cast_assert " "%s->%s (%s:%d:%s)" "\n", 
type, target, file, line, func);
        }
    }
}

down to


static inline void _nocheck__trace_object_dynamic_cast_assert(const char * 
type, const char * target, const char * file, int line, const char * func)
{
    if (trace_event_get_state(TRACE_OBJECT_DYNAMIC_CAST_ASSERT) && 
qemu_loglevel_mask(LOG_TRACE)) {
            qemu_log("object_dynamic_cast_assert " "%s->%s (%s:%d:%s)" "\n", 
type, target, file, line, func);
    }
}

which feels more in keeping with the kind of level of complexity you should
want to be inlined in trace callers.

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