For the patch series' current implementation, the modified
qemu_thread_create()
in 7/7 patch returns a Boolean value to indicate whether it succeeds
and set the
error reason into the passed errp, and did not set the errno.
Actually
another
similar errno-set issue has been talked in last patch. :)
If we set the errno in future qemu_thread_create(), we need to
distinguish the Linux
and Windows implementation. For Linux, we can use error_setg_errno()
to set errno.
But for Windows, I am not sure if we can use
"errno = GetLastError()"
No, that won't work.
to set errno, as this seems a little weird. Do you have any idea
about this?
BTW, if we have a decent errno-set way for Windows, I will adopt
your above
proposal for this patch.
According to MS docs, _beginthreadex() sets errno on failure:
If successful, each of these functions returns a handle to
the newly
created thread; however, if the newly created thread exits too
quickly, _beginthread might not return a valid handle. (See the
discussion in the Remarks section.) On an error, _beginthread
returns -1L, and errno is set to EAGAIN if there are too many
threads, to EINVAL if the argument is invalid or the stack
size is
incorrect, or to EACCES if there are insufficient resources
(such as
memory). On an error, _beginthreadex returns 0, and errno and
_doserrno are set.
https://docs.microsoft.com/cpp/c-runtime-library/reference/beginthread-beginthreadex
Looks like the current code's use of GetLastError() after
_beginthreadex() failure is wrong.
Fix that, and both qemu_thread_create() implementations set errno on
failure, which in turn lets you make qemu_signalfd_compat() and thus
qemu_signalfd() behave sanely regardless of which qemu_thread_create()
implementation they use below the hood.
Thanks for the detail explanation. :)
To fix that, how about replacing the "GetLastError()" with the returned
value "hThread" (actually returns 0)? I mean
...
hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
data, 0, &thread->tid);
if (!hThread) {
if (data->mode != QEMU_THREAD_DETACHED) {
DeleteCriticalSection(&data->cs);
}
error_setg_win32(errp, hThread,
"failed to create win32_start_routine");
g_free(data);
return false;
}
No. On failure, _beginthreadex() returns *zero*, not an error code. It
also sets errno. That's the error code you need to use:
hThread = (HANDLE) _beginthreadex(NULL, 0, win32_start_routine,
data, 0, &thread->tid);
if (!hThread) {
error_setg_errno(errp, errno, "can't create thread");
}
Ok, clearer, we also want the errno message to be printed, along
with _beginthreadex() sets the errno's value by default.
Thanks.
Except I really wouldn't convert qemu_thread_create() to Error! I'd
make it return zero on success, a negative errno code on failure, and
leave the Error business to its callers. Basically, replace
error_exit(err, ...) by return err.
Emm, I am afraid not converting to Error means it is a little bit
trickier to
handle for the callers. Especially for migration callers [see patch 7/7],
they have no initial errp passed, thus error_setg_xx() seems less useful.
Instead in the caller I choose the error_reportf_err(local_err, ...)
to only print
the detail error message and return that caller's original failing tag.
(But for those callers who already have the errp passed, both "return
ret"
and "convert qemu_thread_create() to Error" are fine to me.)
Besides, there is only one caller needs the errno value, that is
qemu_signal_init():
"return -errno". Other callers do not use errno to indicate if it
succeeds.
Thus the current patches choose pass Error to hold the detail error
message and return a bool to indicate if the function succeeds.
Would you like to share your reason for not converting this function
to Error?
[1#begin]
The caller qemu_signalfd_compat() can then do
ret = qemu_thread_create(&thread, "signalfd_compat",
sigwait_compat, info,
QEMU_THREAD_DETACHED);
if (ret < 0) {
errno = ret;
return -1;
}
[1#end]
[2#begin]
A caller that already has an Error **errp parameter could do
ret = qemu_thread_create(...);
if (ret < 0) {
error_setg_errno(errp, -ret, "<error message goes here>");
}
[2#end]
[3#begin]
Callers that want to continue aborting on failure simply do
ret = qemu_thread_create(...);
assert(ret >= 0);
[3#end]
If that turns out to be too much of a bother, you could create a
convenience wrapper for it:
void qemu_thread_create_nofail(QemuThread *thread, const char
*name,
void *(*start_routine)(void*),
void *arg, int mode)
{
int err = qemu_thread_create(thread, name, start_routine,
arg, mode);
if (err) {
error_exit(err, __func__);
}
}
I am wondering the above qemu_thread_create_nofail is a convenience for
"[3#begin] => [3#end]" or "[1#begin] => [3#end]"..
If for "[3#begin] => [3#end]", I'd like to use the xxx_nofail wrapper
as the error
message is more detailed.
If for "[1#begin] => [3#end]", I'd like to explain more for this patch
series: we
want our qemu code to be more robust by not making qemu exit(-1) after
qemu_thread_create() fails and let the callers handle this. E.g. for
hmp/qmp
callers, make qemu abort() seems too violent if they fails.
Have a nice day
Fei