Fei Li <f...@suse.com> writes:
> On 10/11/2018 06:02 PM, Markus Armbruster wrote:
>> Fei Li <f...@suse.com> writes:
>>
>>> Currently, when qemu_signal_init() fails it only returns a non-zero
>>> value but without propagating any Error. But its callers need a
>>> non-null err when runs error_report_err(err), or else 0->msg occurs.
>> The bug is in qemu_init_main_loop():
>>
>> ret = qemu_signal_init();
>> if (ret) {
>> return ret;
>> }
>>
>> Fails without setting an error, unlike the other failures. Its callers
>> crash then.
> Thanks!
Consider working that into your commit message.
>>> To avoid such segmentation fault, add a new Error parameter to make
>>> the call trace to propagate the err to the final caller.
>>>
>>> Signed-off-by: Fei Li <f...@suse.com>
>>> Reviewed-by: Fam Zheng <f...@redhat.com>
>>> ---
>>> include/qemu/osdep.h | 2 +-
>>> util/compatfd.c | 9 ++++++---
>>> util/main-loop.c | 9 ++++-----
>>> 3 files changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>>> index 4f8559e550..f1f56763a0 100644
>>> --- a/include/qemu/osdep.h
>>> +++ b/include/qemu/osdep.h
>>> @@ -421,7 +421,7 @@ struct qemu_signalfd_siginfo {
>>> additional fields in the future) */
>>> };
>>> -int qemu_signalfd(const sigset_t *mask);
>>> +int qemu_signalfd(const sigset_t *mask, Error **errp);
>>> void sigaction_invoke(struct sigaction *action,
>>> struct qemu_signalfd_siginfo *info);
>>> #endif
>>> diff --git a/util/compatfd.c b/util/compatfd.c
>>> index 980bd33e52..d3ed890405 100644
>>> --- a/util/compatfd.c
>>> +++ b/util/compatfd.c
>>> @@ -16,6 +16,7 @@
>>> #include "qemu/osdep.h"
>>> #include "qemu-common.h"
>>> #include "qemu/thread.h"
>>> +#include "qapi/error.h"
>>> #include <sys/syscall.h>
>>> @@ -65,7 +66,7 @@ static void *sigwait_compat(void *opaque)
>>> }
>>> }
>>> -static int qemu_signalfd_compat(const sigset_t *mask)
>>> +static int qemu_signalfd_compat(const sigset_t *mask, Error **errp)
>>> {
>>> struct sigfd_compat_info *info;
>>> QemuThread thread;
>>> @@ -73,11 +74,13 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>>> info = malloc(sizeof(*info));
>>> if (info == NULL) {
>>> + error_setg(errp, "Failed to allocate signalfd memory");
>>> errno = ENOMEM;
>>> return -1;
>>> }
>>> if (pipe(fds) == -1) {
>>> + error_setg(errp, "Failed to create signalfd pipe");
>>> free(info);
>>> return -1;
>>> }
>>> @@ -94,7 +97,7 @@ static int qemu_signalfd_compat(const sigset_t *mask)
>>> return fds[0];
>>> }
>>> -int qemu_signalfd(const sigset_t *mask)
>>> +int qemu_signalfd(const sigset_t *mask, Error **errp)
>>> {
>>> #if defined(CONFIG_SIGNALFD)
>>> int ret;
>>> @@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask)
>>> }
>>> #endif
>>> - return qemu_signalfd_compat(mask);
>>> + return qemu_signalfd_compat(mask, errp);
>>> }
>> I think this takes the Error conversion too far.
>>
>> qemu_signalfd() is like the signalfd() system call, only portable, and
>> setting FD_CLOEXEC. In particular, it reports failure just like a
>> system call: it sets errno and returns -1. I'd prefer to keep it that
>> way. Instead...
>>
>>> diff --git a/util/main-loop.c b/util/main-loop.c
>>> index affe0403c5..9671b6d226 100644
>>> --- a/util/main-loop.c
>>> +++ b/util/main-loop.c
>>> @@ -71,7 +71,7 @@ static void sigfd_handler(void *opaque)
>>> }
>>> }
>>> -static int qemu_signal_init(void)
>>> +static int qemu_signal_init(Error **errp)
>>> {
>>> int sigfd;
>>> sigset_t set;
>>> @@ -94,9 +94,8 @@ static int qemu_signal_init(void)
>>> pthread_sigmask(SIG_BLOCK, &set, NULL);
>>> sigdelset(&set, SIG_IPI);
>>> - sigfd = qemu_signalfd(&set);
>>> + sigfd = qemu_signalfd(&set, errp);
>>> if (sigfd == -1) {
>>> - fprintf(stderr, "failed to create signalfd\n");
>>> return -errno;
>>> }
>>>
>> ... change this function so:
>>
>> pthread_sigmask(SIG_BLOCK, &set, NULL);
>> sigdelset(&set, SIG_IPI);
>> sigfd = qemu_signalfd(&set);
>> if (sigfd == -1) {
>> - fprintf(stderr, "failed to create signalfd\n");
>> + error_setg_errno(errp, errno, "failed to create signalfd");
>> return -errno;
>> }
>>
>> Does this make sense?
> Yes, it looks more concise if we only have this patch, but triggers
> one errno-set
> issue after applying patch 7/7, which adds a Error **errp parameter for
> qemu_thread_create() to let its callers handle the error instead of
> exit() directly
> to add the robustness.
I guess you're talking about the qemu_thread_create() in
qemu_signalfd_compat(). Correct?
> 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.
> Have a nice day, thanks for the review :)
> Fei
>>> @@ -109,7 +108,7 @@ static int qemu_signal_init(void)
>>> #else /* _WIN32 */
>>> -static int qemu_signal_init(void)
>>> +static int qemu_signal_init(Error **errp)
>>> {
>>> return 0;
>>> }
>>> @@ -148,7 +147,7 @@ int qemu_init_main_loop(Error **errp)
>>> init_clocks(qemu_timer_notify_cb);
>>> - ret = qemu_signal_init();
>>> + ret = qemu_signal_init(errp);
>>> if (ret) {
>>> return ret;
>>> }
>>
