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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

> +    } 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;
     }

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

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.

Paolo