On Fri, Jul 18, 2025 at 03:20:35PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berra...@redhat.com> writes:
>
> > On Fri, Jul 18, 2025 at 07:59:50AM +0200, Markus Armbruster wrote:
> >> Markus Armbruster <arm...@redhat.com> writes:
> >>
> >> > Adam Williamson <awill...@redhat.com> 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 <awill...@redhat.com>
> >> >> ---
> >> >> 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.
>
> Correct.
>
> > 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 <stefa...@redhat.com>
> > 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.
>
> Oh yes. We should do this even if we find a reason for keeping
> qemu_log() as it is. The obvious way would be a new function
> qemu_log_with_timestamp().
Now proposed at:
https://lists.nongnu.org/archive/html/qemu-devel/2025-07/msg05234.html
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 :|
