When qemu_signal_init() fails in qemu_init_main_loop(), we return
without setting an error. Its callers crash then when they try to
report the error with error_report_err().
To avoid such segmentation fault, add a new Error parameter to make
the call trace to propagate the err to the final caller.
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Fam Zheng <famz@redhat.com>
Signed-off-by: Fei Li <fli@suse.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
util/main-loop.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/util/main-loop.c b/util/main-loop.c
index affe0403c5..443cb4cfe8 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;
@@ -96,7 +96,7 @@ static int qemu_signal_init(void)
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;
}
@@ -109,7 +109,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 +148,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;
}
--
2.13.7
Fei Li <fli@suse.com> writes:
> When qemu_signal_init() fails in qemu_init_main_loop(), we return
> without setting an error. Its callers crash then when they try to
> report the error with error_report_err().
Yes, that's a bug. Broken in 2f78e491d7b, v2.2.0. Has escaped notice
since qemu_signalfd() is quite unlikely to fail. Could go into 3.1 as a
bug fix, but I think punting it to the next release is just fine.
> To avoid such segmentation fault, add a new Error parameter to make
> the call trace to propagate the err to the final caller.
>
Let's add:
Fixes: 2f78e491d7b46542158ce0b8132ee4e05bc0ade4
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Fam Zheng <famz@redhat.com>
> Signed-off-by: Fei Li <fli@suse.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> ---
> util/main-loop.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/util/main-loop.c b/util/main-loop.c
> index affe0403c5..443cb4cfe8 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;
> @@ -96,7 +96,7 @@ static int qemu_signal_init(void)
> 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;
> }
>
> @@ -109,7 +109,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 +148,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;
> }
Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 11/28/2018 08:53 PM, Markus Armbruster wrote:
> Fei Li <fli@suse.com> writes:
>
>> When qemu_signal_init() fails in qemu_init_main_loop(), we return
>> without setting an error. Its callers crash then when they try to
>> report the error with error_report_err().
> Yes, that's a bug. Broken in 2f78e491d7b, v2.2.0. Has escaped notice
> since qemu_signalfd() is quite unlikely to fail. Could go into 3.1 as a
> bug fix, but I think punting it to the next release is just fine.
Thanks. :) BTW, should I send the next version only includes patch 1/5
and 2/5 separately so that you can merge? (I guess Dave will help to
merge the other three migration related patches)
>
>> To avoid such segmentation fault, add a new Error parameter to make
>> the call trace to propagate the err to the final caller.
>>
> Let's add:
>
> Fixes: 2f78e491d7b46542158ce0b8132ee4e05bc0ade4
ok.
>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Cc: Fam Zheng <famz@redhat.com>
>> Signed-off-by: Fei Li <fli@suse.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>> ---
>> util/main-loop.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/util/main-loop.c b/util/main-loop.c
>> index affe0403c5..443cb4cfe8 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;
>> @@ -96,7 +96,7 @@ static int qemu_signal_init(void)
>> 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;
>> }
>>
>> @@ -109,7 +109,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 +148,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;
>> }
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Thanks again for the review.
Have a nice day
Fei
Fei Li <fli@suse.com> writes: > On 11/28/2018 08:53 PM, Markus Armbruster wrote: >> Fei Li <fli@suse.com> writes: >> >>> When qemu_signal_init() fails in qemu_init_main_loop(), we return >>> without setting an error. Its callers crash then when they try to >>> report the error with error_report_err(). >> Yes, that's a bug. Broken in 2f78e491d7b, v2.2.0. Has escaped notice >> since qemu_signalfd() is quite unlikely to fail. Could go into 3.1 as a >> bug fix, but I think punting it to the next release is just fine. > Thanks. :) BTW, should I send the next version only includes patch 1/5 > and 2/5 separately so that you can merge? (I guess Dave will help to > merge the other three migration related patches) I can pick patches out of a series for merging, and I trust Dave can, too. But keeping unrelated fixes separate is a good idea. I can see three groups: PATCH 1 (main loop), PATCH 2 (thread abstraction), PATCH 3-5 (migration). [...]
On 11/29/2018 04:35 PM, Markus Armbruster wrote: > Fei Li <fli@suse.com> writes: > >> On 11/28/2018 08:53 PM, Markus Armbruster wrote: >>> Fei Li <fli@suse.com> writes: >>> >>>> When qemu_signal_init() fails in qemu_init_main_loop(), we return >>>> without setting an error. Its callers crash then when they try to >>>> report the error with error_report_err(). >>> Yes, that's a bug. Broken in 2f78e491d7b, v2.2.0. Has escaped notice >>> since qemu_signalfd() is quite unlikely to fail. Could go into 3.1 as a >>> bug fix, but I think punting it to the next release is just fine. >> Thanks. :) BTW, should I send the next version only includes patch 1/5 >> and 2/5 separately so that you can merge? (I guess Dave will help to >> merge the other three migration related patches) > I can pick patches out of a series for merging, and I trust Dave can, > too. But keeping unrelated fixes separate is a good idea. I can see > three groups: PATCH 1 (main loop), PATCH 2 (thread abstraction), PATCH > 3-5 (migration). > > [...] Ok, thanks for the detail explanation. I once thought the first two patches are belong to one group, that's why I tried to separate them. But considering too many groups they are belong to and maintainers are willing to pick, I'd like to keep sending them within one patch series in the next version. :) Have a nice day, thanks a lot Fei
© 2016 - 2026 Red Hat, Inc.