On Wed, 19 Oct 2022 16:57:54 +0100
Daniel P. Berrangé <berra...@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 <pbonz...@redhat.com>
> > Signed-off-by: Greg Kurz <gr...@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
