Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when qemu_signal_init fails

[Markus Armbruster]

Fam Zheng <address@hidden> writes:

> On Wed, 09/05 19:20, Fei Li wrote:
>> 
>> 
>> On 09/05/2018 04:36 PM, Daniel P. Berrangé wrote:
>> > 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:
>> > > > 
>> ... snip ...
>> > > > >            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.
>> Sorry for the above example, it is not exactly correct: for the patch3 case,
>> the
>> system will exit in device_set_realized(), where the first error_propagate()
>> is called
>> if we pass errp directly, but not in the final callee.. Sorry for the
>> misleading.
>> 
>> For another example, its call trace:
>> qemu_thread_create(, NULL)
>> <= iothread_complete(, NULL)
>> <== user_creatable_complete(, NULL)
>> <=== object_new_with_propv(, errp)
>> <==== object_new_with_props(, errp) {... error_propagate(errp, local_err);
>> ...}
>> <===== iothread_create(, &error_abort)
>> The exit occurs in object_new_with_props where the first error_propagate is
>> called.
>> 
>> Either the device_set_realized() or object_new_with_props() is a middle
>> caller, thus
>> we can only see the top half stack trace until where error_handle_fatal() is
>> called.
>> 
>> In other words, the exit() occurs neither in the final callee nor the
>> initial caller.
>> Sorry for the misleading example again..
>
> This means using error_propagate can potentially lose the final callee in the
> error_abort cases. That is why it's preferrable to just pass errp down the
> calling chain when possible. The reason why object_new_with_propv uses
> error_propagate is because both object_property_add_child and
> user_creatable_complete return void, thus cannot flag success/failure to its
> caller via their return values. To check whether they succeed,
> object_new_with_propv wants a non-NULL err parameter. But like you said, errp
> passed to object_new_with_propv may or may not be NULL, so a local_err local
> variable is defined to cope with that.
>
> Alternatively it could do this instead:
>
> {
>
>     ...
>
>     if (errp) {
>         object_property_add_child(parent, id, obj, errp);
>         if (*errp) {
>             goto error;
>         }
>     } else {
>         Error *local_err = NULL;
>         object_property_add_child(parent, id, obj, &local_err);
>         if (local_err) {
>             goto error;
>         }
>     }
>     ...
>
> }

Pretty awful, isn't it?

> This way if error_abort was passed and object_property_add_child failed, the
> abort point would be in the innermost function. But this is boilerplate code 
> so
> it's not used.
>
> On the contrary, using error_propagate when not necessary also means more 
> lines
> of code but gives less info on the call trace when aborted.
>
> So I fully agree with Dan.

So do I: simply pass down the errp argument when you can.

Sometimes, you can't because the callee doesn't return a useful value.
As I explained elsewhere (pointed to by Eric), I'd solve that by making
the callee return a useful value.  It's what functions do.

>
> Fam
>
>> > 
>> > > 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.
>> ok.
>> > 
>> > 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
>> Have a nice day, thanks
>> Fei
>>