On Wed, Sep 05, 2018 at 12:17:24PM +0800, Fei Li wrote:
> Thanks for the review! :)
>
>
> On 09/04/2018 07:26 PM, Daniel P. Berrangé wrote:
> > On Tue, Sep 04, 2018 at 07:08:18PM +0800, 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.
> > >
> > > Signed-off-by: Fei Li <f...@suse.com>
> > > ---
> > > include/qemu/osdep.h | 2 +-
> > > util/compatfd.c | 17 ++++++++++++-----
> > > util/main-loop.c | 11 +++++++----
> > > 3 files changed, 20 insertions(+), 10 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..65501de622 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 malloc in %s", __func__);
> > I'd avoid using __func__ in error messages. Instead
> >
> > "Failed to allocate signalfd memory"
> Ok, thanks.
> >
> > > errno = ENOMEM;
> > > return -1;
> > > }
> > > if (pipe(fds) == -1) {
> > > + error_setg(errp, "Failed to create a pipe in %s", __func__);
> > "Failed to create signalfd pipe"
> OK.
> >
> > > free(info);
> > > return -1;
> > > }
> > > @@ -94,17 +97,21 @@ 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;
> > > + Error *local_err = NULL;
> > > +#if defined(CONFIG_SIGNALFD)
> > > ret = syscall(SYS_signalfd, -1, mask, _NSIG / 8);
> > > if (ret != -1) {
> > > qemu_set_cloexec(ret);
> > > return ret;
> > > }
> > > #endif
> > > -
> > > - return qemu_signalfd_compat(mask);
> > > + ret = qemu_signalfd_compat(mask, &local_err);
> > > + if (local_err) {
> > > + error_propagate(errp, local_err);
> > > + }
> > Using a local_err is not required - you can just pass errp stright
> > to qemu_signalfd_compat() and then check
> >
> > if (ret < 0)
> For the use of a local error object & error_propagate call, I'd like to
> explain here. :)
> In our code, the initial caller passes two kinds of Error to the call trace,
> one is
> something like &error_abort and &error_fatal, the other is NULL.
>
> For the former, the exit() occurs in the functions where
> error_handle_fatal() is called
> (e.g. called by error_propagate/error_setg/...). The patch3: qemu_init_vcpu
> is the case,
> that means the system will exit in the final callee: qemu_thread_create(),
> instead of
> the initial caller pc_new_cpu(). In such case, I think propagating seems
> more reasonable.
I don't really agree. It is preferrable to abort immediately at the deepest
place which raises the error. The stack trace will thus show the full call
chain leading upto the problem.
> How do you think passing errp straightly for the latter case, and use a
> local error object &
> error_propagate for the former case? This is a distinct treatment, but would
> shorten the code.
It is inappropriate to second-guess whether the caller is a passing in
NULL or &error_abort, or another Error object. What is passed in can
change at any time in the future.
We should only ever use a local error where the local method has a need
to look at the error contents before returning to the caller. Any other
case should just use the errp directly.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
