Eric Blake <ebl...@redhat.com> writes: > On 11/06/2015 08:36 AM, Markus Armbruster wrote: >> Eric Blake <ebl...@redhat.com> writes: >> >>> By using &error_abort, we can avoid a local err variable in >>> situations where we expect success. It also has the nice >>> effect that if the test breaks, the error message from >>> error_abort tends to be nicer than that of g_assert(). >>> >>> Signed-off-by: Eric Blake <ebl...@redhat.com> >> [Boring mechanical changes snipped...] > >>> pt_copy->type = pt->type; >>> - ops->serialize(pt, &serialize_data, visit_primitive_type, &err); >>> - ops->deserialize((void **)&pt_copy, serialize_data, >>> visit_primitive_type, &err); >>> + ops->serialize(pt, &serialize_data, visit_primitive_type, >>> &error_abort); >>> + ops->deserialize((void **)&pt_copy, serialize_data, >>> visit_primitive_type, >>> + &error_abort); >>> >>> - g_assert(err == NULL); >> >> This looks like a (very minor) bug fix / cleanup: you're not supposed to >> pass the same &err to multiple functions without checking and clearing >> it in between, because the second failure trips assert(*errp == NULL) in >> error_setv(). Harmless here, but it's nice to get rid of a bad example. > > Harmless here because we are asserting that err is still NULL after the > chain (which means it was NULL at all points during the chain). But I > agree that it is nice to get rid of poor practice, and that adding a > paragraph to the commit message to point it out would be a nice idea. > >> >> Suggest to note the cleanup in the commit message. > > We may be close enough to take the series without needing a v11; if > that's the case, and you are the one squashing in the change, how about > this text: > > This patch has an additional bonus of fixing several call sites that > were passing &err to two different functions without checking it in > between. In general that is unsafe practice; because if the first > function sets an error, the second function could abort() if it tries to > set a different error. We got away with it because we were asserting > that err was NULL through the entire chain, but switching to > &error_abort avoids the questionable practice up front.
Works for me.
