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
