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;
}
}
...
}
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.
Fam