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

[Markus Armbruster]

Peter Crosthwaite <address@hidden> writes:

> 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 :)

I'd leave the assertion where it is now.