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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Oct 20, 2022 at 11:49:37AM +0200, Greg Kurz wrote:
> On Thu, 20 Oct 2022 12:21:27 +1000
> Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> > On 10/20/22 01:16, Greg Kurz wrote:
> > 
> > I don't understand why daemonize changes -d tid at all.
> > If there's a bug there, please separate it out.
> > 
> > I don't understand the is_main_log_thread checks.
> > Why is the main thread special?
> > 
> 
> The current code base either opens a per-thread file in
> qemu_log_trylock() when -d tid is enabled, or only a
> single global file in qemu_log_set_internal() in the
> opposite case.
> 
> The goal of this patch is to go through the `In case we
> are a daemon redirect stderr to logfile` logic, so that
> other users of stderr, aka. error_report(), can benefit
> from it as well. Since this is only done for the global
> file, the logic was changed to : _main_ thread to always
> use the global file and other threads to use the per-thread
> file.
> 
> I now realize how terrible a choice this is. It violates
> the current logic too much and brings new problems like
> "how to identify the main thread"...

snip

> > I would have thought that this was the only change required -- ignoring qemu_loglevel when 
> > daemonized.
> > 
> 
> I was thinking the same at first, but this ended up in the
> global file being open with a filename containing a '%d'...
> I chose the direction of doing the g_strdup_printf() trick
> for the global file as well but then I had to make sure
> that qemu_log_trylock() wouldn't try later to open the same
> file, hence the _main_ thread check...
> 
> The question is actually : where stderr should point to in
> the '-daemonize -D foo%d.log -d tid` case ?

I'm tending towards thinking the question is wrong, because
it is imposing semantics on -D that it was never designed to
address.

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.


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.

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.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|