| Message ID | 1363273057-25850-1-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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
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
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; }
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(-)