Re: [Qemu-devel] [PATCH 6/7] qbus_find(): report errors via Error

[Luiz Capitulino]

On Mon, 04 Feb 2013 19:23:15 +0100
Laszlo Ersek <address@hidden> wrote:

> On 02/04/13 18:51, Luiz Capitulino wrote:
> > On Fri,  1 Feb 2013 18:38:18 +0100
> > Laszlo Ersek <address@hidden> wrote:
> > 
> >>
> >> Signed-off-by: Laszlo Ersek <address@hidden>
> > 
> > Splitting this as I suggested in the other patch would make review
> > easier. I honestly got a bit lost while reviewing this one.
> 
> I think you cannot review this series without applying each patch in
> lock-step with the review, and contrasting the next patch with the
> just-updated tree. In that light it's especially regrettable that my
> series didn't apply to master (it certainly did when I posted it).

I'll review it in more detail when you respin.

> 
> Anyway the approach I took was:
> - determine the set of functions to modify,
> - work in a bottom-up fashion,
> - when changing a function signature, change all callers.
> 
> Suppose there are three functions, X, Y, Z, with calls like X->Y, X->Z.
> 
> #1 First I'd fix Y (signature change) and adapt its call site in X --
> acccept the error and print it. At this point X consumes a few errors,
> and some other errors don't reach it at all.

May work for simple cases (although has the minor problem of mixing
signature change with adding new error handling to X), but doesn't scale
when Y is called by more than one function and their error handling differ.

> 
> #2 Then I'd fix Z (signature change) and adapt its call site in X --
> accept the error and print it. At this point Y and Z are OK, and X has
> several places that accept and consume (print/free) errors.
> 
> #3 In the next patch, X's signature is changed, all error consumption /
> printing sites in X are changed to propagate / generate instead, and all
> call sites or X are updated.
> 
> The only patch I could split is #3, but then X would in part consume, in
> part propagate errors. Do you suggest I do that?

The suggestion I gave in another email is to separate signature changes
from error handling changes.

Taking your example above, I'd have the following patches:

 1. Change Y's signature to take an Error ** argument and change
    X to pass NULL for the new argument

 2. Change Z's signature to take an Error ** argument and change
    X to pass NULL for the new argument

 3. Change X to pass an non-NULL Error ** argument to Y and Z and
    handle error accordingly

 4. If Y is non-void and if the return value is success/failure, drop it

 5. If Y is non-void and if the return value is success/failure, drop it

Now, you could merge patches 1 and 2 if the functions are only used
by X. On the other hand, if Y and/or Z are called by other functions,
you'd have to add more patches after 3 to add error handling to them.

I know this is more work, but it makes it easier to review.