Re: [Qemu-devel] [PATCH v2 1/6] error: Add error_abort

[Peter Crosthwaite]

On Fri, Dec 6, 2013 at 9:59 PM, Markus Armbruster <address@hidden> wrote:
> Eric Blake <address@hidden> writes:
>
>> On 12/05/2013 03:13 AM, Markus Armbruster wrote:
>>
>>>>
>>>> For error_propagate, if the destination error is &error_abort, then
>>>> the abort happens at propagation time.
>>>>
>>>> Signed-off-by: Peter Crosthwaite <address@hidden>
>>>> ---
>>>> changed since v1:
>>>> Delayed assertions that *errp == NULL.
>>>
>>> Care to explain why you want to delay these assertions?  I'm not sure I
>>> get it...
>>
>> error_abort as a global variable is always NULL.
>>
>>>
>>> [...]
>>>> @@ -31,7 +33,6 @@ void error_set(Error **errp, ErrorClass err_class, const 
>>>> char *fmt, ...)
>>>>      if (errp == NULL) {
>>>>          return;
>>>>      }
>>>> -    assert(*errp == NULL);
>>
>> So *&error_abort is null and this assertion would fire, unless we delay
>> the check for NULL...

The assertion evaluates to true so there is no issue leaving it where
it is. I am perhaps being overly defensive ...

>
> Err, one of us is confused :)
>
> When errp == &error_abort, then *errp should be null.  If it isn't, then
> something got stored in error_abort, which is quite wrong.  Leaving the
> assertion where it is catches that.
>

In a rather obscure way. Following on from the "what happens if
someone overwrites error_abort" discussion, I was going for the "limp
on" approach when someone stores to error abort. My thinking is that
error abort is potentially corruptable, given it's whole reason to be
is to trap fatally broken code. Hardening it against a bad pointer
deref leading up to the fatal error it actually traps may make sense.
Although I'm probably chasing ghosts there. Happy to revert however
depending on consensus. Both v1 and v2 are of equivalent functionality
in the 99.99% case and I have no strong opinion one way or the other.

Votes welcome :)

Regards,
Peter

>>>>
>>>>      err = g_malloc0(sizeof(*err));
>>>>
>>>> @@ -40,6 +41,12 @@ void error_set(Error **errp, ErrorClass
>>>> err_class, const char *fmt, ...)
>>>>      va_end(ap);
>>>>      err->err_class = err_class;
>>>>
>>>> +    if (errp == &error_abort) {
>>>> +        error_report("%s", error_get_pretty(err));
>>>> +        abort();
>>>> +    }
>>>> +
>>>> +    assert(*errp == NULL);
>>
>> ...until after the check for &error_abort.
>