[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/3] error: don't rely on pointer comparisons
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/3] error: don't rely on pointer comparisons |
Date: |
Wed, 17 Jun 2015 09:41:19 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 |
On 06/17/2015 01:24 AM, Michael S. Tsirkin wrote:
> makes it possible to copy error_abort pointers,
> not just pass them on directly.
>
> @@ -168,7 +175,7 @@ void error_free(Error *err)
>
> void error_propagate(Error **dst_errp, Error *local_err)
> {
> - if (local_err && dst_errp == &error_abort) {
> + if (local_err && error_is_abort(dst_errp)) {
> error_report_err(local_err);
> abort();
> } else if (dst_errp && !*dst_errp) {
As I pointed out on 3/3, this breaks code that does:
if (local_err) {
error_propagate(errp, local_err);
...
}
now that local_err is non-NULL when errp is error_abort. But what if
you alter the semantics, and have error_propagate return a bool (true if
an error was propagated, false if no error or caller didn't care):
bool error_propagate(Error **dst_errp, Error *local_err)
{
if (error_is_abort(&local_err)) {
assert(error_is_abort(dst_errp);
return false;
}
if (local_err && error_is_abort(dst_errp)) {
error_report_err(local_err);
abort();
}
if (dst_errp && !*dst_errp) {
*dst_errp = local_err;
return true;
}
if (local_err) {
error_free(local_err);
}
return false;
}
then callers can be modified to this idiom (also has the benefit of
being one line shorter):
if (error_propagate(errp, local_err)) {
...
}
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature