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

[fei]

> 在 2019年1月9日，22:36，Markus Armbruster <address@hidden> 写道：
> 
> 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.
Ok, thanks for the review :)

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