| Message ID | 20221019151651.334334-2-groug@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | util/log: Always send errors to logfile when daemonized | expand |
On Wed, Oct 19, 2022 at 05:16:50PM +0200, Greg Kurz wrote: > A subsequent patch needs to be able to differentiate the main QEMU > thread from other threads. An obvious way to do so is to compare > log_thread_id() and getpid(), based on the fact that they are equal > for the main thread on systems that have the gettid() syscall (e.g. > linux). > > Adapt the fallback code for systems without gettid() to provide the > same assumption. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > util/log.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/util/log.c b/util/log.c > index d6eb0378c3a3..e1c2535cfcd2 100644 > --- a/util/log.c > +++ b/util/log.c > @@ -72,8 +72,13 @@ static int log_thread_id(void) > #elif defined(SYS_gettid) > return syscall(SYS_gettid); > #else > + static __thread int my_id = -1; > static int counter; > - return qatomic_fetch_inc(&counter); > + > + if (my_id == -1) { > + my_id = getpid() + qatomic_fetch_inc(&counter); > + } > + return my_id; This doesn't look safe for linux-user when we fork, but don't exec. The getpid() will change after the fork, but counter won't be reset, so a thread in the parent could clash with a thread in the forked child. I feel like if we want to check for the main thread, we should be using pthread_self(), and compare result against the value cached from main. Or cache in a __constructor__ function in log.c to keep it isolated from main(). With regards, Daniel
On Wed, 19 Oct 2022 16:57:54 +0100 Daniel P. Berrangé <berrange@redhat.com> wrote: > On Wed, Oct 19, 2022 at 05:16:50PM +0200, Greg Kurz wrote: > > A subsequent patch needs to be able to differentiate the main QEMU > > thread from other threads. An obvious way to do so is to compare > > log_thread_id() and getpid(), based on the fact that they are equal > > for the main thread on systems that have the gettid() syscall (e.g. > > linux). > > > > Adapt the fallback code for systems without gettid() to provide the > > same assumption. > > > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > util/log.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/util/log.c b/util/log.c > > index d6eb0378c3a3..e1c2535cfcd2 100644 > > --- a/util/log.c > > +++ b/util/log.c > > @@ -72,8 +72,13 @@ static int log_thread_id(void) > > #elif defined(SYS_gettid) > > return syscall(SYS_gettid); > > #else > > + static __thread int my_id = -1; > > static int counter; > > - return qatomic_fetch_inc(&counter); > > + > > + if (my_id == -1) { > > + my_id = getpid() + qatomic_fetch_inc(&counter); > > + } > > + return my_id; > > This doesn't look safe for linux-user when we fork, but don't exec. > ... which is a "dangerous" situation if the parent is already multi-threaded at fork() time. The child thread must only call async-signal-safe functions and... > The getpid() will change after the fork, but counter won't be > reset, so a thread in the parent could clash with a thread > in the forked child. > ... pthread_create() isn't one AFAIK. This case has undefined behavior. Anyway, no matter what we do, even with a regular fork+exec pattern, log_thread_id() no longer guarantees unique values for all threads that could be running concurrently (unlike gettid() or counter++), e.g. - parent process with pid A and one extra thread => parent uses thread ids A and A+1 - fork child process with pid B == A+1 - child execs => child uses thread id A+1 > I feel like if we want to check for the main thread, we should > be using pthread_self(), and compare result against the value > cached from main. Or cache in a __constructor__ function in > log.c to keep it isolated from main(). > Hmm... pthread_self() is only guaranteed to be unique within a process. It doesn't look safe either to compare results of pthread_self() from different process contexts. > > With regards, > Daniel Thanks for bringing this corner case up ! It highlights that I should definitely go for another approach that doesn't require to check for the main thread at all. Cheers, -- Greg
On 10/19/22 17:57, Daniel P. Berrangé wrote: >> + if (my_id == -1) { >> + my_id = getpid() + qatomic_fetch_inc(&counter); >> + } >> + return my_id; > This doesn't look safe for linux-user when we fork, but don't exec. Linux-user won't ever get here, however bsd-user might. We should have get_thread_id() somewhere in util/, for example https://github.com/wine-mirror/wine/blob/master/dlls/ntdll/unix/server.c > The getpid() will change after the fork, but counter won't be > reset, so a thread in the parent could clash with a thread > in the forked child. It might clash even if the counter is reset for that matter. Paolo
On Thu, 20 Oct 2022 12:39:41 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 10/19/22 17:57, Daniel P. Berrangé wrote: > >> + if (my_id == -1) { > >> + my_id = getpid() + qatomic_fetch_inc(&counter); > >> + } > >> + return my_id; > > This doesn't look safe for linux-user when we fork, but don't exec. > > Linux-user won't ever get here, however bsd-user might. We should have > get_thread_id() somewhere in util/, for example > > https://github.com/wine-mirror/wine/blob/master/dlls/ntdll/unix/server.c > We have qemu_get_thread_id() already : https://git.qemu.org/?p=qemu.git;a=blob;f=util/oslib-posix.c;h=827a7aadba444cdb128284f5b4ba43934c78c3db;hb=HEAD#l96 > > The getpid() will change after the fork, but counter won't be > > reset, so a thread in the parent could clash with a thread > > in the forked child. > > It might clash even if the counter is reset for that matter. > Yes. > Paolo >
diff --git a/util/log.c b/util/log.c index d6eb0378c3a3..e1c2535cfcd2 100644 --- a/util/log.c +++ b/util/log.c @@ -72,8 +72,13 @@ static int log_thread_id(void) #elif defined(SYS_gettid) return syscall(SYS_gettid); #else + static __thread int my_id = -1; static int counter; - return qatomic_fetch_inc(&counter); + + if (my_id == -1) { + my_id = getpid() + qatomic_fetch_inc(&counter); + } + return my_id; #endif }
A subsequent patch needs to be able to differentiate the main QEMU thread from other threads. An obvious way to do so is to compare log_thread_id() and getpid(), based on the fact that they are equal for the main thread on systems that have the gettid() syscall (e.g. linux). Adapt the fallback code for systems without gettid() to provide the same assumption. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Greg Kurz <groug@kaod.org> --- util/log.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)