Re: [Qemu-devel] [PATCH for-4.0 v9 06/16] qemu_thread: Make qemu_thread_create() handle errors properly

[Markus Armbruster]

Fei Li <address@hidden> writes:

> 在 2019/1/9 上午1:07, Markus Armbruster 写道:
>> fei <address@hidden> writes:
>>
>>>> 在 2019年1月8日，01:18，Markus Armbruster <address@hidden> 写道：
>>>>
>>>> Fei Li <address@hidden> writes:
[...]
>>>>> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
>>>>> index 865e476df5..39834b0551 100644
>>>>> --- a/util/qemu-thread-posix.c
>>>>> +++ b/util/qemu-thread-posix.c
>>>>> @@ -15,6 +15,7 @@
>>>>> #include "qemu/atomic.h"
>>>>> #include "qemu/notify.h"
>>>>> #include "qemu-thread-common.h"
>>>>> +#include "qapi/error.h"
>>>>>
>>>>> static bool name_threads;
>>>>>
>>>>> @@ -500,9 +501,13 @@ static void *qemu_thread_start(void *args)
>>>>>      return r;
>>>>> }
>>>>>
>>>>> -void qemu_thread_create(QemuThread *thread, const char *name,
>>>>> -                       void *(*start_routine)(void*),
>>>>> -                       void *arg, int mode)
>>>>> +/*
>>>>> + * Return a boolean: true/false to indicate whether it succeeds.
>>>>> + * If fails, propagate the error to Error **errp and set the errno.
>>>>> + */
>>>> Let's write something that can pass as a function contract:
>>>>
>>>>    /*
>>>>     * Create a new thread with name @name
>>>>     * The thread executes @start_routine() with argument @arg.
>>>>     * The thread will be created in a detached state if @mode is
>>>>     * QEMU_THREAD_DETACHED, and in a jounable state if it's
>>>>     * QEMU_THREAD_JOINABLE.
>>>>     * On success, return true.
>>>>     * On failure, set @errno, store an error through @errp and return
>>>>     * false.
>>>>     */
>>> Thanks so much for amending! :)
>>>> Personally, I'd return negative errno instead of false, and dispense
>>>> with setting errno.
>>> Emm, I think I have replied this in last version, but due to several 
>>> reasons I did not wait for your feedback and sent the v9. Sorry for that.. 
>>> And I like to paste my two considerations here:
>>> “- Actually only one caller needs the errno, that is the above 
>>> qemu_signalfd_compat().
>> Yes.
>>
>>> - For the returning value, I remember there's once a email thread talking 
>>> about it: returning a bool (and let the passed errp hold the error message) 
>>> is to keep the consistency with glib.
>> GLib doesn't discourage return types other than boolean.  It only asks
>> that if you return boolean, then true should mean success and false
>> should mean failure.  See
>>
>>      https://developer.gnome.org/glib/stable/glib-Error-Reporting.html
>>
>> under "Rules for use of GError", item "By convention, if you return a
>> boolean value".
>>
>> The discussion you remember was about a convention we used to enforce in
>> QEMU, namely to avoid returning boolean success, and return void
>> instead.  That was basically a bad idea.
>>
>>> So IMO I am wondering whether it is really needed to change the bool 
>>> (true/false) to int (0/-errno), just for that sole function: 
>>> qemu_signalfd_compat() which needs the errno. Besides if we return -errno, 
>>> for each caller we need add a local variable like “ret= 
>>> qemu_thread_create()” to store the -errno.
>> Well, you either assign the error code to errno just for that caller, or
>> you return the error code just for that caller.  I'd do the latter
>> because I consider it slightly simpler.  Compare
>>
>>   * On success, return true.
>>   * On failure, set @errno, store an error through @errp and return
>>   * false.
>>
>> to
>>
>>   * On success, return zero.
>>   * On failure, store an error through @errp and return negative errno.
>>
>> where the second sentence describes just two instead of three actions.
>>
>> [...]
> Ok, decribing two actions than three is indeed simpler. But I still
> have one uncertain:
> for those callers do not need the errno value, could we just check the
> return value
> to see whether it is negative, but not cache the unused return value? I mean
>
> In the caller:
>
> {...
>     if (qemu_thread_create() < 0) {// do some cleanup}
> ...}

This is just fine when you handle all errors the same.

> instead of
>
> {    int ret;
> ...
>     ret = qemu_thread_create();
>     if (ret < 0) { //do some cleanup }
>
> ...}

I'd object to this one unless the value of @ret gets used elsewhere.

> As the first one can lessen quite a lot of codes. :)
>
> Have a nice day, thanks
>
> Fei