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