Fei Li <lifei1...@126.com> writes:
> 在 2019/1/9 上午1:07, Markus Armbruster 写道:
>> fei <lifei1...@126.com> writes:
>>
>>>> 在 2019年1月8日，01:18，Markus Armbruster <arm...@redhat.com> 写道：
>>>>
>>>> Fei Li <f...@suse.com> 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
