Re: [PATCH] osdep.h: Add doc comment for qemu_get_thread_id()

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 28 Jul 2020 at 16:17, Eric Blake <eblake@redhat.com> wrote:
>>
>> On 7/16/20 10:41 AM, Peter Maydell wrote:
>> > Add a documentation comment for qemu_get_thread_id(): since this
>> > is rather host-OS-specific it's useful if people writing the
>> > implementation and people thinking of using the function know
>> > what the purpose and limitations are.
>> >
>> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> > ---
>> > Based on conversation with Dan on IRC, and prompted by the recent
>> > patch to add OpenBSD support.
>> >
>> > Q: should we document exactly what the thread-id value is for
>> > each host platform in the QMP documentation ? Somebody writing
>> > a management layer app should ideally not have to grovel through
>> > the application to figure out what they should do with the
>> > integer value they get back from query-cpus...
>> >
>> >   include/qemu/osdep.h | 14 ++++++++++++++
>> >   1 file changed, 14 insertions(+)
>>
>> Do we need a counterpart change...
>>
>> >
>> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> > index 4841b5c6b5f..8279f72e5ed 100644
>> > --- a/include/qemu/osdep.h
>> > +++ b/include/qemu/osdep.h
>> > @@ -515,6 +515,20 @@ bool qemu_has_ofd_lock(void);
>> >
>> >   bool qemu_write_pidfile(const char *pidfile, Error **errp);
>> >
>> > +/**
>> > + * qemu_get_thread_id: Return OS-specific ID of current thread
>> > + *
>> > + * This function returns an OS-specific identifier of the
>> > + * current thread. This will be used for the "thread-id" field in
>> > + * the response to the QMP query-cpus and query-iothreads commands.
>>
>> ...to the qapi definition of query-cpus and query-iothreads?
>
> Well, that was my question above. Currently the QAPI documentation
> says absolutely nothing about what the thread-id values mean
> for any host OS (beyond "ID of the underlying host thread"), which
> means that any management layer application needs to look in the
> implementation to find out what they actually are...

... which they will have to do to actually use it for the purpose we
have in mind, namely:

>> > + * The intention is that a VM management layer application can then
>> > + * use it to tie specific QEMU vCPU and IO threads to specific host
>> > + * CPUs using whatever the host OS's CPU affinity setting API is.
>> > + * New implementations of this function for new host OSes should
>> > + * return the most sensible integer ID that works for that purpose.
>> > + *
>> > + * This function should not be used for anything else inside QEMU.
>> > + */

Do they?

> Improving the QAPI docs would probably be something like:
>  * add a list of host OSes and semantics to the doc comment
>    for CpuInfoFast
>  * add cross-references to that definition from everywhere
>    else in QAPI that uses a thread-id/thread_id
>  * add a comment in the C file to say "if you're adding another
>    OS ifdef here please update the QAPI doc comment"

If they do, then this sounds like a plan.

By the way, the #else case smells:

    #else
        return getpid();
    #endif

The PID is quite unlikely to be "an OS-specific identifier of the
current thread".  Shouldn't we fail instead of lie when we don't know
how to compute the truth?