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