Re: [Qemu-devel] [PATCH v4] log: Make glib logging go through QEMU

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2876 bytes --]

On Mon, Jan 14, 2019 at 12:54:20PM +0100, Christophe Fergeau wrote:
> On Fri, Jan 04, 2019 at 03:40:34PM +0000, Stefan Hajnoczi wrote:
> > On Thu, Jan 03, 2019 at 03:14:55PM +0100, Christophe Fergeau wrote:
> > > Hey,
> > > 
> > > On Thu, Jan 03, 2019 at 10:54:25AM +0000, Stefan Hajnoczi wrote:
> > > > On Fri, Dec 14, 2018 at 11:56:42AM +0100, Christophe Fergeau wrote:
> > > > > +static void qemu_log_func(const gchar *log_domain,
> > > > > +                          GLogLevelFlags log_level,
> > > > > +                          const gchar *message,
> > > > > +                          gpointer user_data)
> > > > > +{
> > > > > +    switch (log_level & G_LOG_LEVEL_MASK) {
> > > > > +    case G_LOG_LEVEL_DEBUG:
> > > > > +        /* Use same G_MESSAGES_DEBUG logic as glib to enable/disable debug
> > > > > +         * messages
> > > > > +         */
> > > > > +        if (qemu_glog_domains == NULL) {
> > > > > +            break;
> > > > > +        }
> > > > > +        if (strcmp(qemu_glog_domains, "all") != 0 &&
> > > > > +          (log_domain == NULL || !strstr(qemu_glog_domains, log_domain))) {
> > > > > +            break;
> > > > > +        }
> > > > > +        /* Fall through */
> > > > > +    case G_LOG_LEVEL_INFO:
> > > > > +        /* Fall through */
> > > > > +    case G_LOG_LEVEL_MESSAGE:
> > > > > +        info_report("%s: %s", log_domain, message);
> > > > 
> > > > QEMU itself uses glib, so what happens if *_report() emit more log
> > > > messages?  Can this result in an infinite loop?
> > > 
> > > If *_report try to output messages through the g_log API, glib will
> > > catch it and will abort, so not an infinite loop, but not a
> > > desirable outcome either.
> > > 
> > > Some low-level logging functions which are going to dump the message
> > > without glib (or using g_log_default_handler()) are needed.
> > > The timestamping/prepending of 'error:', 'warning:' to the error message
> > > is done right below *_report (in vreport) so this looked like a good
> > > place for that.
> > 
> > If you're confident that QEMU will never (indirectly) emit a glib log
> > message from the *_report() code path, then I'm happy.
> 
> In these codepaths, I'm confident up to
> void monitor_vfprintf(FILE *stream, const char *fmt, va_list ap)
> {
>     if (cur_mon && !monitor_cur_is_qmp()) {
>         monitor_vprintf(cur_mon, fmt, ap);
>     } else {
>         vfprintf(stream, fmt, ap);
>     }
> }
> 
> From there, it was not obvious to me whether the monitor_vprintf
> codepath can be triggered from info/warn/error_report or not. If they
> can, I'd feel less confident that monitor_vprintf will never indirectly
> call glib log functions.

I think we're okay because otherwise monitor_vprintf() ->
info/warn/error_report() would already be an infinite loop today.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]