Re: [PATCH v6 13/27] util/log: add missing error reporting in qemu_log_trylock_with_err

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> One codepath that could return NULL failed to populate the errp
> object.
>
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  util/log.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/util/log.c b/util/log.c
> index 1644e6814b..1c9c7adb2d 100644
> --- a/util/log.c
> +++ b/util/log.c
> @@ -118,6 +118,7 @@ static FILE *qemu_log_trylock_with_err(Error **errp)
   static FILE *qemu_log_trylock_with_err(Error **errp)
   {
       FILE *logfile;

       logfile = thread_file;
       if (!logfile) {

Slow path: no log file cached for this thread.

           if (log_per_thread) {

We're logging to per-thread log files.  @global_filename is a file name
pattern; derive this thread's log file from it.

               g_autofree char *filename
                   = g_strdup_printf(global_filename, log_thread_id());
               logfile = fopen(filename, "w");
               if (!logfile) {
                   error_setg_errno(errp, errno,
                                    "Error opening logfile %s for thread %d",
                                    filename, log_thread_id());
                   return NULL;
               }
               thread_file = logfile;
               qemu_log_thread_cleanup_notifier.notify = qemu_log_thread_cleanup;
               qemu_thread_atexit_add(&qemu_log_thread_cleanup_notifier);
           } else {

Logging to a single log file.  Fetch it from @global_file.

               rcu_read_lock();
               /*
                * FIXME: typeof_strip_qual, as used by qatomic_rcu_read,
                * does not work with pointers to undefined structures,
                * such as we have with struct _IO_FILE and musl libc.
                * Since all we want is a read of a pointer, cast to void**,
                * which does work with typeof_strip_qual.
                */
>              logfile = qatomic_rcu_read((void **)&global_file);
>              if (!logfile) {

@global_file was null when we fetched it.

It can be null when @daemonized and the user didn't set a log file.

>                  rcu_read_unlock();
> +                error_setg(errp, "Global log file output is not open");

Impact?  The only callers passing non-null @errp is
qemu_set_log_internal(), and it can call only when @log_per_thread.  We
can't reach the new error then.

So this is just a latent bug.  Worth mentioning in the commit message?

>                  return NULL;
>              }

We do not set thread_file.  We'll take the slow path again.  Harmless
enough, but its a bit at odds with how the code is structured here.
Observation, not a demand.

>          }
       }

       qemu_flockfile(logfile);
       return logfile;
   }

Reviewed-by: Markus Armbruster <armbru@redhat.com>