Re: [PATCH] util/log: Always send errors to logfile when daemonized

[Greg Kurz <groug@kaod.org>]

On Fri, 14 Oct 2022 10:51:36 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 10/14/22 08:08, Greg Kurz wrote:
> > 
> > +        need_to_open_file = log_flags && !per_thread;
> 
> Pre-existing, but I think this should check log_per_thread instead of 
> per_thread.
> 

Yes I agree, and also check qemu_loglevel instead of log_flags for
the same reason (and to match the comment just above).

> > +    } else if (filename) {
> > +        /*
> > +         * If we are daemonized, we will only log if there is a filename.
> > +         */
> > +        need_to_open_file = true;
> 
> Slightly nicer:
> 
>      } else {
>         /*
>          * If daemonized, always log to the -D file if present.
>          */
>          need_to_open_file = filename != NULL;
>      }
> 

Sure.

> > @@ -271,10 +276,22 @@ static bool qemu_set_log_internal(const char *filename, bool changed_name,
> >   
> >       if (!logfile && need_to_open_file) {
> >           if (filename) {
> > -            logfile = fopen(filename, log_append ? "a" : "w");
> > +            g_autofree char *fname = NULL;
> > +
> > +            /*
> > +             * If per-thread, filename contains a single %d that should be
> > +             * converted.
> > +             */
> > +            if (per_thread) {
> > +                fname = g_strdup_printf(filename, getpid());
> > +            } else {
> > +                fname = g_strdup(filename);
> > +            }
> > +
> > +            logfile = fopen(fname, log_append ? "a" : "w");
> >               if (!logfile) {
> >                   error_setg_errno(errp, errno, "Error opening logfile %s",
> > -                                 filename);
> > +                                 fname);
> >                   return false;
> >               }
> >               /* In case we are a daemon redirect stderr to logfile */
> 
> This could conflict with the file opened by qemu_log_trylock() when 
> per-thread logging is enabled *and* QEMU is daemonized.  Perhaps 
> something like:
> 

Yeah... if the main thread happens to call qemu_log(), it then opens
a file with the same name indeed. Thanks for catching that !

> 1) change qemu_log_trylock() to
> 
> -        if (log_per_thread) {
> +        if (log_per_thread && log_thread_id() != getpid()) {
> 
> i.e. use the global_file for the main thread
> 
> 2) change qemu_log_unlock() to
> 
> -        if (!log_per_thread) {
> +        if (!thread_file) {
> 
> to match (1)
> 
> 3) change log_thread_id() to something like
> 
> ...
> #else
>      static __thread int my_id = -1;
>      static int counter;
>      if (my_id == -1) {
>          my_id = getpid() + qatomic_fetch_inc(&counter);
>      }
>      return my_id;
> #endif
> 
> and perhaps do a dummy trylock/unlock late in qemu_set_log_internal(), 
> to ensure that the main thread is the one with log_thread_id() == getpid()?
> 
> I think this can be a separate patch before this one.
> 

2) and 3) can certainly be preparatory work but I think 1)
should be squashed in my patch. Because of the !per_thread
check in need_to_open_file, the existing code in
qemu_set_log_internal() doesn't even open the global file
and qemu_log_trylock() would always return NULL for the
main thread.

Thanks for the quick answer and suggestions !

> Paolo
>