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 <f...@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