On Fri, Dec 6, 2013 at 9:59 PM, Markus Armbruster <arm...@redhat.com> wrote: > Eric Blake <ebl...@redhat.com> 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 <peter.crosthwa...@xilinx.com> >>>> --- >>>> 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. >