On 11/06/2015 08:36 AM, Markus Armbruster wrote: > Eric Blake 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 > [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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org