On Thu, 20 Oct 2022 12:52:21 +0200
Paolo Bonzini <pbonz...@redhat.com> wrote:
> 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.
>
I agree about the semantics of -D but the implementation forces
the logging to go through stderr connected to a file in the
daemonize case... so in the end -D controls where stderr
is connected to, and this affects any other non-log user of
stderr.
> (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,
This is exactly why I decided to try this approach.
> 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().
>
static int get_unix_tid(void)
{
int ret = -1;
#ifdef HAVE_PTHREAD_GETTHREADID_NP
ret = pthread_getthreadid_np();
#elif defined(linux)
ret = syscall( __NR_gettid );
#elif defined(__sun)
ret = pthread_self();
#elif defined(__APPLE__)
ret = mach_thread_self();
mach_port_deallocate(mach_task_self(), ret);
#elif defined(__NetBSD__)
ret = _lwp_self();
#elif defined(__FreeBSD__)
long lwpid;
thr_self( &lwpid );
ret = lwpid;
#elif defined(__DragonFly__)
ret = lwp_gettid();
#endif
return ret;
}
We could import all these cases except the defined(linux) case maybe since
it should be covered by what we already have in log_thread_id() :
#ifdef CONFIG_GETTID
return gettid();
#elif defined(SYS_gettid)
return syscall(SYS_gettid);
> > 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");
>
+1
> 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).
>
I had missed that but yes indeed... I'll fix that in a preparatory
patch.
> Paolo
>
Thanks Paolo !
--
Greg
