Re: [Qemu-devel] [PATCH] qemu-sockets: Fix assertion failure

[Paolo Bonzini <pbonzini@redhat.com>]

Il 06/03/2013 16:59, Markus Armbruster ha scritto:
>> >
>> > I don't really understand the difference. As long as the function
>> > doesn't depend on the Error object to be present (which it doesn't),
>> > isn't it semantically exactly the same?
> I guess it is in this case (I didn't call your patch wrong).  However, I
> figure that as soon as we go beyond utterly trivial with errors, it's
> advisable to put them in local variables, and error_propagate() to the
> parameter only at the end.  Messing around with errp parameters directly
> feels error-prone, and I'm afraid even correct messing could easily lead
> to incorrect messing, when folks imitate it without really understanding
> what they're doing.

Yes, that's my thought more or less.  However, the possibilities are:

1) asserting that there is no error.  We hardly ever do this, and it's
not clear to me that we should start.  It would introduce one more
error-propagation idiom, and more confusion.

2) do nothing if there is an error.  This is what for example the QAPI
visitors do.

3) what Laszlo suggested.  However, it is almost certainly a bug to call
this function with a pre-existing error, because this function has quite
"strong" side effects.  Hence this might mask a bug and leave pending
connections.

4) just your patch.  This is just as wrong as (3) if there was a
pre-existing error, and it also overwrites the pre-existing error;
harder to debug, and inconsistent with functions using the local variables.


None is optimal, but (2) seems the best.

Paolo

> Yes, extra local variables make the error propagation code even bulkier.
> No, I don't like it any more than you do.