[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/6] error: Add error_abort
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/6] error: Add error_abort |
Date: |
Fri, 06 Dec 2013 14:16:31 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) |
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.
[Qemu-devel] [PATCH v2 2/6] hw/core/qdev: Delete dead code, Peter Crosthwaite, 2013/12/04
[Qemu-devel] [PATCH v2 3/6] hw: Remove assert_no_error usages, Peter Crosthwaite, 2013/12/04
[Qemu-devel] [PATCH v2 4/6] target-i386: Remove assert_no_error usage, Peter Crosthwaite, 2013/12/04
[Qemu-devel] [PATCH v2 5/6] qemu-option: Remove qemu_opts_create_nofail, Peter Crosthwaite, 2013/12/04
[Qemu-devel] [PATCH v2 6/6] qerror: Remove assert_no_error(), Peter Crosthwaite, 2013/12/04