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

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

[Kevin Wolf]

inet_connect_opts() tries all possible addrinfos returned by
getaddrinfo(). If one fails with an error, the next one is tried. In
this case, the Error should be discarded because the whole operation is
successful if another addrinfo from the list succeeds; and if it
doesn't, setting an already set Error will trigger an assertion failure.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 util/qemu-sockets.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 1350ccc..32e609a 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -373,6 +373,14 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
     }
 
     for (e = res; e != NULL; e = e->ai_next) {
+
+        /* Overwriting errors isn't allowed, so clear any error that may have
+         * occured in the previous iteration */
+        if (error_is_set(errp)) {
+            error_free(*errp);
+            *errp = NULL;
+        }
+
         if (connect_state != NULL) {
             connect_state->current_addr = e;
         }
-- 
1.8.1.4

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

[Paolo Bonzini]

Il 06/03/2013 11:48, Kevin Wolf ha scritto:
> inet_connect_opts() tries all possible addrinfos returned by
> getaddrinfo(). If one fails with an error, the next one is tried. In
> this case, the Error should be discarded because the whole operation is
> successful if another addrinfo from the list succeeds; and if it
> doesn't, setting an already set Error will trigger an assertion failure.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  util/qemu-sockets.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 1350ccc..32e609a 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -373,6 +373,14 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
>      }
>  
>      for (e = res; e != NULL; e = e->ai_next) {
> +
> +        /* Overwriting errors isn't allowed, so clear any error that may have
> +         * occured in the previous iteration */
> +        if (error_is_set(errp)) {
> +            error_free(*errp);
> +            *errp = NULL;
> +        }
> +
>          if (connect_state != NULL) {
>              connect_state->current_addr = e;
>          }
> 

Should we also do nothing if errp is not NULL on entry?

Paolo

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

[Kevin Wolf]

Am 06.03.2013 um 12:04 hat Paolo Bonzini geschrieben:
> Il 06/03/2013 11:48, Kevin Wolf ha scritto:
> > inet_connect_opts() tries all possible addrinfos returned by
> > getaddrinfo(). If one fails with an error, the next one is tried. In
> > this case, the Error should be discarded because the whole operation is
> > successful if another addrinfo from the list succeeds; and if it
> > doesn't, setting an already set Error will trigger an assertion failure.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  util/qemu-sockets.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index 1350ccc..32e609a 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -373,6 +373,14 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
> >      }
> >  
> >      for (e = res; e != NULL; e = e->ai_next) {
> > +
> > +        /* Overwriting errors isn't allowed, so clear any error that may have
> > +         * occured in the previous iteration */
> > +        if (error_is_set(errp)) {
> > +            error_free(*errp);
> > +            *errp = NULL;
> > +        }
> > +
> >          if (connect_state != NULL) {
> >              connect_state->current_addr = e;
> >          }
> > 
> 
> Should we also do nothing if errp is not NULL on entry?

We could assert(!error_is_set(errp)) if we wanted. As soon as you've got
an Error, you must return instead of calling more functions with the
same error pointer.

Kevin

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

[Laszlo Ersek]

On 03/06/13 12:11, Kevin Wolf wrote:
> Am 06.03.2013 um 12:04 hat Paolo Bonzini geschrieben:
>> Il 06/03/2013 11:48, Kevin Wolf ha scritto:
>>> inet_connect_opts() tries all possible addrinfos returned by
>>> getaddrinfo(). If one fails with an error, the next one is tried. In
>>> this case, the Error should be discarded because the whole operation is
>>> successful if another addrinfo from the list succeeds; and if it
>>> doesn't, setting an already set Error will trigger an assertion failure.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  util/qemu-sockets.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>>> index 1350ccc..32e609a 100644
>>> --- a/util/qemu-sockets.c
>>> +++ b/util/qemu-sockets.c
>>> @@ -373,6 +373,14 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
>>>      }
>>>  
>>>      for (e = res; e != NULL; e = e->ai_next) {
>>> +
>>> +        /* Overwriting errors isn't allowed, so clear any error that may have
>>> +         * occured in the previous iteration */
>>> +        if (error_is_set(errp)) {
>>> +            error_free(*errp);
>>> +            *errp = NULL;
>>> +        }
>>> +
>>>          if (connect_state != NULL) {
>>>              connect_state->current_addr = e;
>>>          }
>>>
>>
>> Should we also do nothing if errp is not NULL on entry?
> 
> We could assert(!error_is_set(errp)) if we wanted. As soon as you've got
> an Error, you must return instead of calling more functions with the
> same error pointer.

I think Luiz would suggest (*) to receive any error into a
NULL-initialized local_err pointer; do the logic above on local_err, and
just before returning, error_propagate() it to errp.

(*) I hope you can see what I did there: if you disagree, you get to
take that to Luiz, even though he didn't say anything. I'm getting
better at working this list! :)

Laszlo

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

[Paolo Bonzini]

Il 06/03/2013 15:46, Laszlo Ersek ha scritto:
>> > We could assert(!error_is_set(errp)) if we wanted. As soon as you've got
>> > an Error, you must return instead of calling more functions with the
>> > same error pointer.
> I think Luiz would suggest (*) to receive any error into a
> NULL-initialized local_err pointer; do the logic above on local_err, and
> just before returning, error_propagate() it to errp.
> 
> (*) I hope you can see what I did there: if you disagree, you get to
> take that to Luiz, even though he didn't say anything. I'm getting
> better at working this list! :)

I agree with Laszlo.

Paolo

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

[Kevin Wolf]

Am 06.03.2013 um 16:04 hat Paolo Bonzini geschrieben:
> Il 06/03/2013 15:46, Laszlo Ersek ha scritto:
> >> > We could assert(!error_is_set(errp)) if we wanted. As soon as you've got
> >> > an Error, you must return instead of calling more functions with the
> >> > same error pointer.
> > I think Luiz would suggest (*) to receive any error into a
> > NULL-initialized local_err pointer; do the logic above on local_err, and
> > just before returning, error_propagate() it to errp.
> > 
> > (*) I hope you can see what I did there: if you disagree, you get to
> > take that to Luiz, even though he didn't say anything. I'm getting
> > better at working this list! :)
> 
> I agree with Laszlo.

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?

Also, Markus' reply makes me think that I should restrict myself to code
areas where errors are reported as -errno. That one I understand at
least...

Kevin

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

[Laszlo Ersek]

On 03/06/13 16:19, Kevin Wolf wrote:
> Am 06.03.2013 um 16:04 hat Paolo Bonzini geschrieben:
>> Il 06/03/2013 15:46, Laszlo Ersek ha scritto:
>>>>> We could assert(!error_is_set(errp)) if we wanted. As soon as you've got
>>>>> an Error, you must return instead of calling more functions with the
>>>>> same error pointer.
>>> I think Luiz would suggest (*) to receive any error into a
>>> NULL-initialized local_err pointer; do the logic above on local_err, and
>>> just before returning, error_propagate() it to errp.
>>>
>>> (*) I hope you can see what I did there: if you disagree, you get to
>>> take that to Luiz, even though he didn't say anything. I'm getting
>>> better at working this list! :)
>>
>> I agree with Laszlo.
> 
> 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?

The difference is when the caller passes in an already set Error. In
this case you release that and replace it with your own error.

Usually we stick to the first error encountered. Under the above
suggestion you'd keep error handling internal to yourself, and in the
end make one attempt to propagate it outwards. If the caller has passed
in NULL, the error is dropped. If the caller's passed in a preexistent
error, then that one takes precedence and the new one is dropped (but it
doesn't interfere with the internal logic). Third, the caller can even
accept your error.

error_propagate() and error_set() deal with the overwrite attempt
differently. The former silently drops the newcomer, whereas the latter
assert()s.

Of course one wonders why a caller would pass in a preexistent Error.

Laszlo

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

[Kevin Wolf]

Am 06.03.2013 um 16:38 hat Laszlo Ersek geschrieben:
> On 03/06/13 16:19, Kevin Wolf wrote:
> > Am 06.03.2013 um 16:04 hat Paolo Bonzini geschrieben:
> >> Il 06/03/2013 15:46, Laszlo Ersek ha scritto:
> >>>>> We could assert(!error_is_set(errp)) if we wanted. As soon as you've got
> >>>>> an Error, you must return instead of calling more functions with the
> >>>>> same error pointer.
> >>> I think Luiz would suggest (*) to receive any error into a
> >>> NULL-initialized local_err pointer; do the logic above on local_err, and
> >>> just before returning, error_propagate() it to errp.
> >>>
> >>> (*) I hope you can see what I did there: if you disagree, you get to
> >>> take that to Luiz, even though he didn't say anything. I'm getting
> >>> better at working this list! :)
> >>
> >> I agree with Laszlo.
> > 
> > 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?
> 
> The difference is when the caller passes in an already set Error. In
> this case you release that and replace it with your own error.
> 
> Usually we stick to the first error encountered. Under the above
> suggestion you'd keep error handling internal to yourself, and in the
> end make one attempt to propagate it outwards. If the caller has passed
> in NULL, the error is dropped. If the caller's passed in a preexistent
> error, then that one takes precedence and the new one is dropped (but it
> doesn't interfere with the internal logic). Third, the caller can even
> accept your error.
> 
> error_propagate() and error_set() deal with the overwrite attempt
> differently. The former silently drops the newcomer, whereas the latter
> assert()s.
> 
> Of course one wonders why a caller would pass in a preexistent Error.

Thanks, Laszlo, now I think I understand what Paolo and you were
suggesting.

However, I'd call any such caller buggy and don't feel like adding code
so that it doesn't break. This is what I meant when I said you should
return when you get an error, and not call other functions with the
already used error pointer.

Kevin

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

[Laszlo Ersek]

On 03/06/13 16:47, Kevin Wolf wrote:
> Am 06.03.2013 um 16:38 hat Laszlo Ersek geschrieben:

>> Of course one wonders why a caller would pass in a preexistent Error.
> 
> Thanks, Laszlo, now I think I understand what Paolo and you were
> suggesting.
> 
> However, I'd call any such caller buggy and don't feel like adding code
> so that it doesn't break. This is what I meant when I said you should
> return when you get an error, and not call other functions with the
> already used error pointer.

I don't disagree. I'm certainly not blocking your patch! :)

Laszlo

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 06.03.2013 um 16:04 hat Paolo Bonzini geschrieben:
>> Il 06/03/2013 15:46, Laszlo Ersek ha scritto:
>> >> > We could assert(!error_is_set(errp)) if we wanted. As soon as you've got
>> >> > an Error, you must return instead of calling more functions with the
>> >> > same error pointer.
>> > I think Luiz would suggest (*) to receive any error into a
>> > NULL-initialized local_err pointer; do the logic above on local_err, and
>> > just before returning, error_propagate() it to errp.
>> > 
>> > (*) I hope you can see what I did there: if you disagree, you get to
>> > take that to Luiz, even though he didn't say anything. I'm getting
>> > better at working this list! :)
>> 
>> I agree with Laszlo.
>
> 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, extra local variables make the error propagation code even bulkier.
No, I don't like it any more than you do.

> Also, Markus' reply makes me think that I should restrict myself to code
> areas where errors are reported as -errno. That one I understand at
> least...

I wish I could, but I stepped into the error swamp long ago, and the
muck is still sticking to my heels.

Whenever a simple error reporting mechanism such as non-null/null or
non-negative/-errno suffices, I very much recommend to use it.

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

[Paolo Bonzini]

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.

[Qemu-devel] [RFC PATCH] qemu-socket: Use local error variable

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
After rebasing this I saw that Anthony already committed a fix that is
very close to my v1. I don't intend to actually change that code, but as
I've already done this, just for comparison what it would look like with
error propagation. Is this what you meant? I find the result more
confusing, to be honest.
---
 util/qemu-sockets.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 3f12296..f231e9c 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -359,6 +359,7 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
     int sock = -1;
     bool in_progress;
     ConnectState *connect_state = NULL;
+    Error *local_err = NULL;
 
     res = inet_parse_connect_opts(opts, errp);
     if (!res) {
@@ -373,15 +374,17 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
     }
 
     for (e = res; e != NULL; e = e->ai_next) {
-        if (error_is_set(errp)) {
-            error_free(*errp);
-            *errp = NULL;
-        }
+
+        Error *ret_err = NULL;
+
         if (connect_state != NULL) {
             connect_state->current_addr = e;
         }
-        sock = inet_connect_addr(e, &in_progress, connect_state, errp);
-        if (in_progress) {
+        sock = inet_connect_addr(e, &in_progress, connect_state, &ret_err);
+        if (error_is_set(&ret_err)) {
+            error_propagate(&local_err, ret_err);
+        } else if (in_progress) {
+            error_free(local_err);
             return sock;
         } else if (sock >= 0) {
             /* non blocking socket immediate success, call callback */
@@ -393,6 +396,11 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
     }
     g_free(connect_state);
     freeaddrinfo(res);
+    if (sock >= 0) {
+        error_free(local_err);
+    } else {
+        error_propagate(errp, local_err);
+    }
     return sock;
 }
 
-- 
1.8.1.4

Re: [Qemu-devel] [RFC PATCH] qemu-socket: Use local error variable

[Laszlo Ersek]

On 03/14/13 15:57, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> After rebasing this I saw that Anthony already committed a fix that is
> very close to my v1. I don't intend to actually change that code, but as
> I've already done this, just for comparison what it would look like with
> error propagation. Is this what you meant? I find the result more
> confusing, to be honest.

I think what I had in mind was:
- I was okay with the logic change you suggested in your v1, just
- turn *errp accesses into local_err accesses,
- when returning, propagate the latter to the former.

The logic seemed OK, I just suggested to keep the massage internal to
the function, only try to propagate it outwards at return time. IOW,
never read *errp.

Laszlo

Re: [Qemu-devel] [RFC PATCH] qemu-socket: Use local error variable

[Kevin Wolf]

Am 14.03.2013 um 16:52 hat Laszlo Ersek geschrieben:
> On 03/14/13 15:57, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > After rebasing this I saw that Anthony already committed a fix that is
> > very close to my v1. I don't intend to actually change that code, but as
> > I've already done this, just for comparison what it would look like with
> > error propagation. Is this what you meant? I find the result more
> > confusing, to be honest.
> 
> I think what I had in mind was:
> - I was okay with the logic change you suggested in your v1, just
> - turn *errp accesses into local_err accesses,
> - when returning, propagate the latter to the former.
> 
> The logic seemed OK, I just suggested to keep the massage internal to
> the function, only try to propagate it outwards at return time. IOW,
> never read *errp.

So you would have used my local_err, but not ret_err? I don't think that
would make it much better, ret_err is actually the nice part.

Kevin

Re: [Qemu-devel] [RFC PATCH] qemu-socket: Use local error variable

[Laszlo Ersek]

On 03/15/13 09:37, Kevin Wolf wrote:
> Am 14.03.2013 um 16:52 hat Laszlo Ersek geschrieben:
>> On 03/14/13 15:57, Kevin Wolf wrote:
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>> After rebasing this I saw that Anthony already committed a fix that is
>>> very close to my v1. I don't intend to actually change that code, but as
>>> I've already done this, just for comparison what it would look like with
>>> error propagation. Is this what you meant? I find the result more
>>> confusing, to be honest.
>>
>> I think what I had in mind was:
>> - I was okay with the logic change you suggested in your v1, just
>> - turn *errp accesses into local_err accesses,
>> - when returning, propagate the latter to the former.
>>
>> The logic seemed OK, I just suggested to keep the massage internal to
>> the function, only try to propagate it outwards at return time. IOW,
>> never read *errp.
> 
> So you would have used my local_err, but not ret_err?

Something like that, yes.

> I don't think that
> would make it much better,

Not contesting that ;)

> ret_err is actually the nice part.

Anyway I'm not feeling strongly about this and I don't want to waste
your time with it. It was just a note in passing. (... Which I should
probably refrain from, lest I waste people's time.)

L.

Re: [Qemu-devel] [RFC PATCH] qemu-socket: Use local error variable

[Kevin Wolf]

Am 15.03.2013 um 17:55 hat Laszlo Ersek geschrieben:
> On 03/15/13 09:37, Kevin Wolf wrote:
> > Am 14.03.2013 um 16:52 hat Laszlo Ersek geschrieben:
> >> On 03/14/13 15:57, Kevin Wolf wrote:
> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >>> ---
> >>> After rebasing this I saw that Anthony already committed a fix that is
> >>> very close to my v1. I don't intend to actually change that code, but as
> >>> I've already done this, just for comparison what it would look like with
> >>> error propagation. Is this what you meant? I find the result more
> >>> confusing, to be honest.
> >>
> >> I think what I had in mind was:
> >> - I was okay with the logic change you suggested in your v1, just
> >> - turn *errp accesses into local_err accesses,
> >> - when returning, propagate the latter to the former.
> >>
> >> The logic seemed OK, I just suggested to keep the massage internal to
> >> the function, only try to propagate it outwards at return time. IOW,
> >> never read *errp.
> > 
> > So you would have used my local_err, but not ret_err?
> 
> Something like that, yes.
> 
> > I don't think that
> > would make it much better,
> 
> Not contesting that ;)
> 
> > ret_err is actually the nice part.
> 
> Anyway I'm not feeling strongly about this and I don't want to waste
> your time with it. It was just a note in passing. (... Which I should
> probably refrain from, lest I waste people's time.)

I'm not going to change this instance anyway now that Anthony pushed his
own fix instead of mine.

However this won't be the last time that I have to deal with an Error
object, so I thought I'd check what is good practice. Seems no such
thing has established yet, which is an answer, even though not the one I
was hoping for.

Kevin

Re: [Qemu-devel] [RFC PATCH] qemu-socket: Use local error variable

[Laszlo Ersek]

On 03/15/13 18:55, Kevin Wolf wrote:

> However this won't be the last time that I have to deal with an Error
> object, so I thought I'd check what is good practice. Seems no such
> thing has established yet, which is an answer, even though not the one I
> was hoping for.

What I've gathered from discussions with Luiz and Markus, there is
indeed no official Error*-handling-style.

FWIW personally I think that my suggestion was quite close to a good
(I'd even hazard "elegant") approach. I did notice that it would look
terrible in the function at hand if applied directly (I actually started
to code it up as an "illustrative patch"). For the emergent fugliness I
blamed inet_connect_opts()'s current structure (several exit points,
transfer of ownership without documentation, etc).

So for the illustration I would have had to restructure the function.
That in turn would have depended on me understanding the non-trivial
life cycle (ownership) of "connect_state" / "res" under the different
return conditions. (That is, when we bail out due to "in progress", the
"connect_state" and the rest of the addrinfo list is:
- either referenced elsewhere,
- or freed,
- or leaked currently.)

I didn't (don't) have time/energy for that -- my bad.

In general, murky ownership transfers seem to be characteristic of qemu.
When a function allocates dynamic memory, it should:
(1) either free it unconditionally (temp working space),
(2) free it on error, return it on success (constructor),
(3) transfer the ownership by function call (huge comment or telling
function name). This includes any refcount increments by the callee.

... The function name "inet_connect_addr" tells us nothing about
qemu_set_fd_handler2() transferring the ownership of "connect_state"
(and the off-hanging addrinfo list) to the global "io_handlers".

inet_connect_opts
  inet_connect_addr
    qemu_set_fd_handler2
      ownership transfer in one case
  release stuff in two other cases

Thanks,
Laszlo

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

[Luiz Capitulino]

On Wed, 06 Mar 2013 15:46:45 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 03/06/13 12:11, Kevin Wolf wrote:
> > Am 06.03.2013 um 12:04 hat Paolo Bonzini geschrieben:
> >> Il 06/03/2013 11:48, Kevin Wolf ha scritto:
> >>> inet_connect_opts() tries all possible addrinfos returned by
> >>> getaddrinfo(). If one fails with an error, the next one is tried. In
> >>> this case, the Error should be discarded because the whole operation is
> >>> successful if another addrinfo from the list succeeds; and if it
> >>> doesn't, setting an already set Error will trigger an assertion failure.
> >>>
> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >>> ---
> >>>  util/qemu-sockets.c | 8 ++++++++
> >>>  1 file changed, 8 insertions(+)
> >>>
> >>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> >>> index 1350ccc..32e609a 100644
> >>> --- a/util/qemu-sockets.c
> >>> +++ b/util/qemu-sockets.c
> >>> @@ -373,6 +373,14 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
> >>>      }
> >>>  
> >>>      for (e = res; e != NULL; e = e->ai_next) {
> >>> +
> >>> +        /* Overwriting errors isn't allowed, so clear any error that may have
> >>> +         * occured in the previous iteration */
> >>> +        if (error_is_set(errp)) {
> >>> +            error_free(*errp);
> >>> +            *errp = NULL;
> >>> +        }
> >>> +
> >>>          if (connect_state != NULL) {
> >>>              connect_state->current_addr = e;
> >>>          }
> >>>
> >>
> >> Should we also do nothing if errp is not NULL on entry?
> > 
> > We could assert(!error_is_set(errp)) if we wanted. As soon as you've got
> > an Error, you must return instead of calling more functions with the
> > same error pointer.
> 
> I think Luiz would suggest (*) to receive any error into a
> NULL-initialized local_err pointer; do the logic above on local_err, and
> just before returning, error_propagate() it to errp.

Yes, I'd suggest that but it turns out that inet_connect_addr() error
reporting was and still is confusing, which causes callers to use it
incorrectly.

This patch (which has been applied by Anthony) solves the problem at
hand but it also introduces a new issue: errors from inet_connect_addr()
are only reported if they happen in the last loop interaction. Note that
a few other errors other than 'couldn't connect' can happen.

Laszlo's comment seemed to have triggered a discussion around Error **,
but this really has very little to do with it: the real problem is that
inet_connect_addr() is too confusing.

inet_connect_addr() has two users: inet_connect_opts() and wait_for_connect(),
with this patch both of them are now ignoring errors from inet_connect_addr().

Suggested solution: refactor inet_connect_addr() to return an errno value.
Callers use error_set() when they want to report an error upward.

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

[Kevin Wolf]

Am 19.03.2013 um 21:34 hat Luiz Capitulino geschrieben:
> On Wed, 06 Mar 2013 15:46:45 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
> > On 03/06/13 12:11, Kevin Wolf wrote:
> > > Am 06.03.2013 um 12:04 hat Paolo Bonzini geschrieben:
> > >> Il 06/03/2013 11:48, Kevin Wolf ha scritto:
> > >>> inet_connect_opts() tries all possible addrinfos returned by
> > >>> getaddrinfo(). If one fails with an error, the next one is tried. In
> > >>> this case, the Error should be discarded because the whole operation is
> > >>> successful if another addrinfo from the list succeeds; and if it
> > >>> doesn't, setting an already set Error will trigger an assertion failure.
> > >>>
> > >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > >>> ---
> > >>>  util/qemu-sockets.c | 8 ++++++++
> > >>>  1 file changed, 8 insertions(+)
> > >>>
> > >>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > >>> index 1350ccc..32e609a 100644
> > >>> --- a/util/qemu-sockets.c
> > >>> +++ b/util/qemu-sockets.c
> > >>> @@ -373,6 +373,14 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
> > >>>      }
> > >>>  
> > >>>      for (e = res; e != NULL; e = e->ai_next) {
> > >>> +
> > >>> +        /* Overwriting errors isn't allowed, so clear any error that may have
> > >>> +         * occured in the previous iteration */
> > >>> +        if (error_is_set(errp)) {
> > >>> +            error_free(*errp);
> > >>> +            *errp = NULL;
> > >>> +        }
> > >>> +
> > >>>          if (connect_state != NULL) {
> > >>>              connect_state->current_addr = e;
> > >>>          }
> > >>>
> > >>
> > >> Should we also do nothing if errp is not NULL on entry?
> > > 
> > > We could assert(!error_is_set(errp)) if we wanted. As soon as you've got
> > > an Error, you must return instead of calling more functions with the
> > > same error pointer.
> > 
> > I think Luiz would suggest (*) to receive any error into a
> > NULL-initialized local_err pointer; do the logic above on local_err, and
> > just before returning, error_propagate() it to errp.
> 
> Yes, I'd suggest that but it turns out that inet_connect_addr() error
> reporting was and still is confusing, which causes callers to use it
> incorrectly.
> 
> This patch (which has been applied by Anthony)

No, Anthony applied a different, but similar patch of his own. This is
why I don't feel particularly responsible for the specific problem any
more.

How to do error handling with Error right is the only reason for me to
continue the discussion.

> solves the problem at
> hand but it also introduces a new issue: errors from inet_connect_addr()
> are only reported if they happen in the last loop interaction. Note that
> a few other errors other than 'couldn't connect' can happen.

> Laszlo's comment seemed to have triggered a discussion around Error **,
> but this really has very little to do with it: the real problem is that
> inet_connect_addr() is too confusing.

Maybe we need to discuss first what the intended behaviour even is. My
interpretation was this: We may have several addresses to try. If one of
them works, the function as a whole has succeeded and must not return an
error, neither in errp nor as -errno. If none of them succeeds, the
function has to return an error, and returning the error of the last
attempt is as good as the error of any other attempt.

> inet_connect_addr() has two users: inet_connect_opts() and wait_for_connect(),
> with this patch both of them are now ignoring errors from inet_connect_addr().
> 
> Suggested solution: refactor inet_connect_addr() to return an errno value.
> Callers use error_set() when they want to report an error upward.

Doesn't change the problem that you need to know when to set a return
value != 0. So it doesn't help, but you'd lose some error information.

Kevin

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

[Luiz Capitulino]

On Wed, 20 Mar 2013 09:39:34 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 19.03.2013 um 21:34 hat Luiz Capitulino geschrieben:
> > On Wed, 06 Mar 2013 15:46:45 +0100
> > Laszlo Ersek <lersek@redhat.com> wrote:
> > 
> > > On 03/06/13 12:11, Kevin Wolf wrote:
> > > > Am 06.03.2013 um 12:04 hat Paolo Bonzini geschrieben:
> > > >> Il 06/03/2013 11:48, Kevin Wolf ha scritto:
> > > >>> inet_connect_opts() tries all possible addrinfos returned by
> > > >>> getaddrinfo(). If one fails with an error, the next one is tried. In
> > > >>> this case, the Error should be discarded because the whole operation is
> > > >>> successful if another addrinfo from the list succeeds; and if it
> > > >>> doesn't, setting an already set Error will trigger an assertion failure.
> > > >>>
> > > >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > >>> ---
> > > >>>  util/qemu-sockets.c | 8 ++++++++
> > > >>>  1 file changed, 8 insertions(+)
> > > >>>
> > > >>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > > >>> index 1350ccc..32e609a 100644
> > > >>> --- a/util/qemu-sockets.c
> > > >>> +++ b/util/qemu-sockets.c
> > > >>> @@ -373,6 +373,14 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
> > > >>>      }
> > > >>>  
> > > >>>      for (e = res; e != NULL; e = e->ai_next) {
> > > >>> +
> > > >>> +        /* Overwriting errors isn't allowed, so clear any error that may have
> > > >>> +         * occured in the previous iteration */
> > > >>> +        if (error_is_set(errp)) {
> > > >>> +            error_free(*errp);
> > > >>> +            *errp = NULL;
> > > >>> +        }
> > > >>> +
> > > >>>          if (connect_state != NULL) {
> > > >>>              connect_state->current_addr = e;
> > > >>>          }
> > > >>>
> > > >>
> > > >> Should we also do nothing if errp is not NULL on entry?
> > > > 
> > > > We could assert(!error_is_set(errp)) if we wanted. As soon as you've got
> > > > an Error, you must return instead of calling more functions with the
> > > > same error pointer.
> > > 
> > > I think Luiz would suggest (*) to receive any error into a
> > > NULL-initialized local_err pointer; do the logic above on local_err, and
> > > just before returning, error_propagate() it to errp.
> > 
> > Yes, I'd suggest that but it turns out that inet_connect_addr() error
> > reporting was and still is confusing, which causes callers to use it
> > incorrectly.
> > 
> > This patch (which has been applied by Anthony)
> 
> No, Anthony applied a different, but similar patch of his own. This is
> why I don't feel particularly responsible for the specific problem any
> more.
> 
> How to do error handling with Error right is the only reason for me to
> continue the discussion.

I think we need a kind of "Error best practices" doc.

> > solves the problem at
> > hand but it also introduces a new issue: errors from inet_connect_addr()
> > are only reported if they happen in the last loop interaction. Note that
> > a few other errors other than 'couldn't connect' can happen.
> 
> > Laszlo's comment seemed to have triggered a discussion around Error **,
> > but this really has very little to do with it: the real problem is that
> > inet_connect_addr() is too confusing.
> 
> Maybe we need to discuss first what the intended behaviour even is. My
> interpretation was this: We may have several addresses to try. If one of
> them works, the function as a whole has succeeded and must not return an
> error, neither in errp nor as -errno. If none of them succeeds, the
> function has to return an error, and returning the error of the last
> attempt is as good as the error of any other attempt.

I agree. When I looked at the code yesterday I had the impression that
several other errors where possible, which made me wonder if we shouldn't
stop short the loop on non-"can't connect" type of errors.

But looking at it again we have only socket() and connect() calls, and
I'd expect that most (all?) non can't connect errors will happen in all
loop iterations, which will cause the error to be reported.

> > inet_connect_addr() has two users: inet_connect_opts() and wait_for_connect(),
> > with this patch both of them are now ignoring errors from inet_connect_addr().
> > 
> > Suggested solution: refactor inet_connect_addr() to return an errno value.
> > Callers use error_set() when they want to report an error upward.
> 
> Doesn't change the problem that you need to know when to set a return
> value != 0. So it doesn't help, but you'd lose some error information.

My real point is that it's easier to check against errno to find out
the error cause (compared to using Error for that).

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

[Kevin Wolf]

Am 20.03.2013 um 13:57 hat Luiz Capitulino geschrieben:
> On Wed, 20 Mar 2013 09:39:34 +0100
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
> > Am 19.03.2013 um 21:34 hat Luiz Capitulino geschrieben:
> > > inet_connect_addr() has two users: inet_connect_opts() and wait_for_connect(),
> > > with this patch both of them are now ignoring errors from inet_connect_addr().
> > > 
> > > Suggested solution: refactor inet_connect_addr() to return an errno value.
> > > Callers use error_set() when they want to report an error upward.
> > 
> > Doesn't change the problem that you need to know when to set a return
> > value != 0. So it doesn't help, but you'd lose some error information.
> 
> My real point is that it's easier to check against errno to find out
> the error cause (compared to using Error for that).

You mean if the caller has to distinguish between different error codes?
I think I would agree that avoiding Error can be a good way then if it
doesn't lose error information. If we would lose information, using
error classes other than generic would be acceptable, right?

In the specific case, I don't think the callers make any difference and
all errors are just errors, so this is mostly about the theory.

Kevin

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

[Luiz Capitulino]

On Wed, 20 Mar 2013 14:37:59 +0100
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 20.03.2013 um 13:57 hat Luiz Capitulino geschrieben:
> > On Wed, 20 Mar 2013 09:39:34 +0100
> > Kevin Wolf <kwolf@redhat.com> wrote:
> > 
> > > Am 19.03.2013 um 21:34 hat Luiz Capitulino geschrieben:
> > > > inet_connect_addr() has two users: inet_connect_opts() and wait_for_connect(),
> > > > with this patch both of them are now ignoring errors from inet_connect_addr().
> > > > 
> > > > Suggested solution: refactor inet_connect_addr() to return an errno value.
> > > > Callers use error_set() when they want to report an error upward.
> > > 
> > > Doesn't change the problem that you need to know when to set a return
> > > value != 0. So it doesn't help, but you'd lose some error information.
> > 
> > My real point is that it's easier to check against errno to find out
> > the error cause (compared to using Error for that).
> 
> You mean if the caller has to distinguish between different error codes?

Yes.

> I think I would agree that avoiding Error can be a good way then if it
> doesn't lose error information. If we would lose information, using
> error classes other than generic would be acceptable, right?

Yes, I think so.

I mean, even to me it's not entirely clear when new classes should be added.
The rule I had in mind was that a new class should be added when a QMP
client needs to distinguish a specific error. However, we're considering
QEMU subsystems to be QMP clients, which (taken to an extreme) would lead us
to our recent past where Error was trying to replace errno.

Markus once wrote about where to set boundaries between errno and Error, but I
don't remember if it was a private discussion or an email to the list. It's
time to write this down in docs/.

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 06.03.2013 um 12:04 hat Paolo Bonzini geschrieben:
>> Il 06/03/2013 11:48, Kevin Wolf ha scritto:
>> > inet_connect_opts() tries all possible addrinfos returned by
>> > getaddrinfo(). If one fails with an error, the next one is tried. In
>> > this case, the Error should be discarded because the whole operation is
>> > successful if another addrinfo from the list succeeds; and if it
>> > doesn't, setting an already set Error will trigger an assertion failure.
>> > 
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> >  util/qemu-sockets.c | 8 ++++++++
>> >  1 file changed, 8 insertions(+)
>> > 
>> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>> > index 1350ccc..32e609a 100644
>> > --- a/util/qemu-sockets.c
>> > +++ b/util/qemu-sockets.c
>> > @@ -373,6 +373,14 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
>> >      }
>> >  
>> >      for (e = res; e != NULL; e = e->ai_next) {
>> > +
>> > +        /* Overwriting errors isn't allowed, so clear any error that may have
>> > +         * occured in the previous iteration */
>> > +        if (error_is_set(errp)) {
>> > +            error_free(*errp);
>> > +            *errp = NULL;
>> > +        }
>> > +
>> >          if (connect_state != NULL) {
>> >              connect_state->current_addr = e;
>> >          }
>> > 
>> 
>> Should we also do nothing if errp is not NULL on entry?
>
> We could assert(!error_is_set(errp)) if we wanted. As soon as you've got
> an Error, you must return instead of calling more functions with the
> same error pointer.

I wouldn't bother asserting this just here.  It's a pervasive issue.  We
already got an assert() that covers actual error overwrites, in
error_set().

[Qemu-devel] Error ** parameter conventions (was: [PATCH] qemu-sockets: Fix assertion failure)

[Markus Armbruster]

[Note cc: Luiz]

Kevin Wolf <kwolf@redhat.com> writes:

> inet_connect_opts() tries all possible addrinfos returned by
> getaddrinfo(). If one fails with an error, the next one is tried. In
> this case, the Error should be discarded because the whole operation is
> successful if another addrinfo from the list succeeds; and if it
> doesn't, setting an already set Error will trigger an assertion failure.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  util/qemu-sockets.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 1350ccc..32e609a 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -373,6 +373,14 @@ int inet_connect_opts(QemuOpts *opts, Error **errp,
>      }
>  
>      for (e = res; e != NULL; e = e->ai_next) {
> +
> +        /* Overwriting errors isn't allowed, so clear any error that may have
> +         * occured in the previous iteration */
> +        if (error_is_set(errp)) {
> +            error_free(*errp);
> +            *errp = NULL;
> +        }
> +
>          if (connect_state != NULL) {
>              connect_state->current_addr = e;
>          }

I'm not sure this is the proper solution, but that could be just my
uncertainty on proper Error usage.

The convention as I understand it is that a function stores whatever
error it wants to return through its errp parameter, with
error_set(errp, ...).  error_set() does nothing when passed a null errp.
This lets callers ignore errors without fuss.

I guess we don't want callers to pass a non-null errp with *errp != NULL
ever, but I don't think that's spelled out anywhere.  I suspect code is
generally unprepared for such usage.

Your patch *clears* errors through its errp parameter.  I'm not saying
it's wrong, I just spooks poor ignorant me.

My gut feeling: as soon as we go beyond utterly trivial with errors,
it's best to put them in local variables, and error_propagate() to the
parameter only at the end.

Let's review some usage patterns, in the hope of learning more.

    /*
     * Do something on @arg.
     * On error, set *@errp if @errp isn't null.
     */
    void do_something(Argument *arg, Error **errp);

    // ignoring errors:
    do_something(arg, NULL);

    // checking and handling errors locally:
    Error *local_err = NULL;
    do_something(arg, &local_err);
    if (local_err) {    // some prefer error_is_set(local_err) here,
                        // but I think that only muddies the waters
        // error handling goes here
        error_free(local_err);
    }

    // propagating to caller via parameter Error *errp:
    Error *local_err = NULL;
    do_something(arg, &local_err);
    if (local_err) {
        // local error handling goes here
        error_propagate(errp, local_err);
    }

    // same, but "simpler" (except it doesn't work):
    do_something(arg, errp);
    if (error_is_set(errp)) {   // BUG: doesn't work when !errp
        // local error handling goes here
    }

    // same, when no local error handling is wanted:
    do_something(arg, errp);

    // but it works only once:
    do_something(arg1, errp);
    do_something(arg2, errp);  // BUG: trips assert() when both fail
    // pity

Let me stress: beware of error_is_set()!  If the argument may or may not
be null, the result is *worthless*.  If it can't be null, you could just
as well test the argument directly.  If it must be null, the result is
always false.

More patterns:
    
    // accumulating errors, first error wins
    Error *err = NULL;
    while (...) {
        Error *local_err = NULL:
        do_something(arg, &local_err);
        if (local_err) {
            // local error handling goes here
            error_propagate(&err, local_err);
        }
    }
    // err contains the first error
    // need to free or propagate it

    // same, but "simpler" (except it doesn't work)
    Error *err = NULL;
    while (...) {
        do_something(arg, &err);// BUG: trips assert() on second error
        if (err) {              // BUG: always true after first error
            // local error handling goes here
        }
    }

    // can omit err when propagating, just replace it by errp:
    // (works because error_propagate() is designed for this)
    while (...) {
        Error *local_err = NULL:
        do_something(arg, &local_err);
        if (local_err) {
            // local error handling goes here
            error_propagate(errp, local_err);
        }
    }
    // errp contains the first error

    // accumulating errors, last error wins
    Error *err = NULL;
    while (...) {
        Error *local_err = NULL:
        do_something(arg, &local_err);
        if (local_err) {
            // local error handling goes here
            // if we do the following often, we should perhaps provide a
            // function for it, just like error_propagate(), only "last
            // one wins" instead of "first one wins"
            if (err) {
                error_free(err);
            }
            err = local_err;
        }
    }
    // err contains the last error
    // need to free or propagate it

    // can omit err when propagating, but need to be careful, because
    // errp may be null:
    while (...) {
        Error *local_err = NULL:
        do_something(arg, &local_err);
        if (local_err) {
            // local error handling goes here
            if (error_is_set(errp)) {
                // exploits that error_is_set(errp) implies errp != NULL
                error_free(*errp);
                *errp = NULL;
            }
            error_propagate(errp, local_err);
        }
    }

Just found a tolerable use of error_is_set().  I'd prefer a function
error_clear(errp) to clear through a possibly null errp.

Still more patterns:

    // try a number of args, until one works
    Error *local_err = NULL;
    for (arg = first_one(); arg; arg = next_one()) {
        if (local_err) {
            error_free(local_err);
            local_err = NULL;
        }
        do_something(arg, &local_err);
        if (!local_err) {
            break;              // arg worked
        }
        // local error handling goes here
    }
    if (local_err) {
        // local error handling goes here
        // need to free or propagate local_err
    } else {
        // arg worked
    }

We can't omit local_err when propagating, because with just errp, we
can't test whether do_something() succeeded.

With a function that return a value that can be tested:

    /*
     * Get @arg's foo.
     * On success, return foo.
     * On failure, return null, and set *@errp if @errp isn't null.
     */
    Foo *get_foo(Argument *arg, Error *errp)

we can omit local_err:

    // try a number of args, until one works
    for (arg = first_one(); arg; arg = next_one()) {
        if (error_is_set(errp)) {
            // exploits that error_is_set(errp) implies errp != NULL
            error_free(*errp);
            *errp = NULL;
        }
        foo = get_foo(arg, errp);
        if (foo) {
            break;              // arg worked
        }
        // local error handling goes here
    }
    if (foo) {
        // arg worked
    } else {
        // local error handling goes here
        // need to free or propagate local_err
    }

This is roughly how your patch works.

Functions like get_foo() spook me, because when I do

    local_err = NULL;
    foo = get_foo(arg, &local_err);
    if (!foo) {
        // fail
        return;
    }
    // success
    // use foo (surely not null)

I worry about local_err being null at // fail, or non-null (and leaked)
at // success.  When I do

    local_err = NULL;
    foo = get_foo(arg, &local_err);
    if (local_err) {
        // fail
        return;
    }
    // success
    // use foo

I worry about foo being null at // success, or non-null (and leaked) at
// fail.

Perhaps I'm too easily worried.