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

[Paolo Bonzini <pbonzini@redhat.com>]

On 10/20/22 11:58, Daniel P. Berrangé wrote:
> 
> The '-d' option enables logging in QEMU, primary for things
> related to TCG. By default that logging goes to stderr, but
> it can be sent to 1 or mnay files, using -D. IOW, -D is NOT
> about controlling where stderr/out is connected. It is
> about where TCG logging goes.

(Aside: it's not just TCG logging.  The default tracing backend is also 
printing to -D).

> Separately, IIUC, you found that when using -daemonize any
> error_report() messages end up in the void, because stderr
> is connected to /dev/null.
> 
> This patch is thus attempting to repurpose -D as a way to
> say where error_report() messages end up with daemonized,
> and this creates the complexity  because -D was never
> intended to be a mechanism to control stderr or error_report
> output.

True, but it already does that if "-d" is specified, because "-d" 
*intentionally* reopens stderr when -daemonize is specified.  So I think 
overall the idea of "make -D always open the destination when 
daemonizing" is sound, the only weird thing is the interaction with "-d 
tid" which is fixed if we just replace the fallback case from 
log_thread_id() as in Wine's get_unix_tid() code.  "-d tid" can just be 
forbidden if the platform is not supported by get_unix_tid().

> If we want to connect stdout/err to something when daemonized
> then lets either have a dedicated option for that, or simply
> tell apps not to use -daemonize and to take care of daemonzing
> themselves, thus having full control over stdout/err. The latter
> is what libvirt uses, because we actually want stderr/out on a
> pipe, not a file, in order to enforce rollover.

I would gladly get rid of -daemonize, unfortunately it has many users. 
Adding further complication to it is not beautiful, but overall I think 
Greg's patch does make sense.  In particular I would continue the 
refactoring by moving


             /*
              * 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);
             }

             return fopen(fname, log_append ? "a" : "w");

to a new function that can be used in both qemu_log_trylock() and 
qemu_set_log_internal().  (In fact this refactoring is a bugfix because 
per-thread log files do not currently obey log_append).

Paolo