| Message ID | 20180907133902.28462-2-fli@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu_thread_create: propagate errors to callers to check | expand |
On Fri, 09/07 21:38, Fei Li wrote: > 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. > > To avoid such segmentation fault, add a new Error parameter to make > the call trace to propagate the err to the final caller. > > This patch also adds the omitted error handling when creating signalfd > pipe fails in qemu_signalfd_compat(). > > Signed-off-by: Fei Li <fli@suse.com> > --- > include/qemu/osdep.h | 2 +- > util/compatfd.c | 9 ++++++--- > util/main-loop.c | 10 +++++----- > 3 files changed, 12 insertions(+), 9 deletions(-) > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > index a91068df0e..09ed85fcb8 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; Extend the context: ret = syscall(SYS_signalfd, -1, mask, _NSIG / 8); if (ret != -1) { qemu_set_cloexec(ret); return ret; } This error path need error_setg() as well. > @@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask) > } > #endif > > - return qemu_signalfd_compat(mask); > + return qemu_signalfd_compat(mask, errp); > } > diff --git a/util/main-loop.c b/util/main-loop.c > index affe0403c5..22aa2b1007 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,10 +94,10 @@ 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"); This fprintf is not needed any more. > - return -errno; > + return -1; > } > > fcntl_setfl(sigfd, O_NONBLOCK); > @@ -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 > Fam
On 09/12/2018 03:55 PM, Fam Zheng wrote: > On Fri, 09/07 21:38, Fei Li wrote: >> 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. >> >> To avoid such segmentation fault, add a new Error parameter to make >> the call trace to propagate the err to the final caller. >> >> This patch also adds the omitted error handling when creating signalfd >> pipe fails in qemu_signalfd_compat(). >> >> Signed-off-by: Fei Li <fli@suse.com> >> --- >> include/qemu/osdep.h | 2 +- >> util/compatfd.c | 9 ++++++--- >> util/main-loop.c | 10 +++++----- >> 3 files changed, 12 insertions(+), 9 deletions(-) >> >> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h >> index a91068df0e..09ed85fcb8 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; > Extend the context: > > ret = syscall(SYS_signalfd, -1, mask, _NSIG / 8); > if (ret != -1) { > qemu_set_cloexec(ret); > return ret; > } > > This error path need error_setg() as well. Thanks for the comment. :) Like adding as below? if (ret != -1) { qemu_set_cloexec(ret); return ret; + } else { + error_setg(errp, "syscall SYS_signalfd failed: %s", strerror(errno)); } #endif return qemu_signalfd_compat(mask, errp); If yes, I'd like to confirm the logic: - 1.1 if the syscall() succeeds, return its value which is not -1: none error; - if fails[1], continue to run the below "qemu_signalfd_compat(mask, errp);" =2.1 if qemu_signalfd_compat() succeeds, the error msg of [1] will be kept in **errp, although the return value indicates a success; =2.2 if qemu_signalfd_compat() fails, the error msg[1] is replaced by another error[2] in the failing function which indicates a failure. Do you mean for the above 2.1, we still need keep the error message? > >> @@ -106,5 +109,5 @@ int qemu_signalfd(const sigset_t *mask) >> } >> #endif >> >> - return qemu_signalfd_compat(mask); >> + return qemu_signalfd_compat(mask, errp); >> } >> diff --git a/util/main-loop.c b/util/main-loop.c >> index affe0403c5..22aa2b1007 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,10 +94,10 @@ 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"); > This fprintf is not needed any more. Ok. Have a nice day Fei > >> - return -errno; >> + return -1; >> } >> >> fcntl_setfl(sigfd, O_NONBLOCK); >> @@ -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 >> > Fam > >
On Thu, 09/13 16:46, Fei Li wrote: > > > On 09/12/2018 03:55 PM, Fam Zheng wrote: > > On Fri, 09/07 21:38, Fei Li wrote: > > > 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. > > > > > > To avoid such segmentation fault, add a new Error parameter to make > > > the call trace to propagate the err to the final caller. > > > > > > This patch also adds the omitted error handling when creating signalfd > > > pipe fails in qemu_signalfd_compat(). > > > > > > Signed-off-by: Fei Li <fli@suse.com> > > > --- > > > include/qemu/osdep.h | 2 +- > > > util/compatfd.c | 9 ++++++--- > > > util/main-loop.c | 10 +++++----- > > > 3 files changed, 12 insertions(+), 9 deletions(-) > > > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > > > index a91068df0e..09ed85fcb8 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; > > Extend the context: > > > > ret = syscall(SYS_signalfd, -1, mask, _NSIG / 8); > > if (ret != -1) { > > qemu_set_cloexec(ret); > > return ret; > > } > > > > This error path need error_setg() as well. > Thanks for the comment. :) > Like adding as below? > > if (ret != -1) { > qemu_set_cloexec(ret); > return ret; > + } else { > + error_setg(errp, "syscall SYS_signalfd failed: %s", > strerror(errno)); > } > #endif > > return qemu_signalfd_compat(mask, errp); > > > If yes, I'd like to confirm the logic: Oh, I missed the fact that qemu_signalfd_compat is a fallback if qemu_set_cloexec() fails. So no, your original patch is good, ignore what I said, there's no need to add the extra error_setg here. If you do so and qemu_signalfd_compat() fails, error_setg() in it will hit the assertion failure. Fam
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index a91068df0e..09ed85fcb8 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); } diff --git a/util/main-loop.c b/util/main-loop.c index affe0403c5..22aa2b1007 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,10 +94,10 @@ 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; + return -1; } fcntl_setfl(sigfd, O_NONBLOCK); @@ -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; }
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. To avoid such segmentation fault, add a new Error parameter to make the call trace to propagate the err to the final caller. This patch also adds the omitted error handling when creating signalfd pipe fails in qemu_signalfd_compat(). Signed-off-by: Fei Li <fli@suse.com> --- include/qemu/osdep.h | 2 +- util/compatfd.c | 9 ++++++--- util/main-loop.c | 10 +++++----- 3 files changed, 12 insertions(+), 9 deletions(-)