Eric Blake <ebl...@redhat.com> writes:
> 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)) {
> ...
> }
Caution! The condition you need to test here is "an error has been
stored into local_err", *not* "an error was propagated". Different when
errp is NULL and local_err has an error.
