On 02/05/13 13:20, Luiz Capitulino wrote: > On Tue, 05 Feb 2013 01:31:14 +0100 > Laszlo Ersek <ler...@redhat.com> wrote: > >> On 02/04/13 18:27, Luiz Capitulino wrote: >>> >>> I usually split this kind of patch the following way: >>> >>> 1. add an Error ** argument to the function reporting the error. Callers >>> are changed to pass NULL for the new argument >> >> If the called function already switches to error_set() / >> error_propagate() at this point, then standing at this patch in a >> possible bisection, the error message is lost. (The caller doesn't print >> it *yet*, the callee doesn't print it *any longer*.) > > If I got what you meant, the called function shouldn't be using error_set() > at this point, as it doesn't even take an Error ** argument. > >> If, OTOH, the called function still prints the error message here, and >> doesn't pass it up (only its signature has changed), then: >> >>> 2. Handling of the new error is added for each caller in a different >>> patch (it's OK to group callers when the error handling is the same) >> >> at some point there will be callers that need the callee to pass up the >> error, and other callers (the ones not yet converted) that want the >> callee not to. > > Yes, but it's case-by-case. For example, if the called function prints to > the monitor or uses fprintf(), then it's OK to keep them until all callers > are converted. > > Now, if the called function reports error with qerror_report() then > we have different options. You could call qerror_report_err() at some > point in the call stack, for example. > > But, honestly speaking, I think I went too general on my feedback and > lost sight of the details concerning this patch and I don't my feedback > is good enough at this point, sorry for that. I'll try to be more > specific when you respin.
OK, let me post v2 soon. I don't expect it to be final, but hopefully it'll enable us to discuss it in more targeted details. Thanks much! Laszlo