Re: [Qemu-devel] [RFC 03/29] io: fix qio_channel_socket_accept err handling

[Peter Xu]

On Mon, Jul 31, 2017 at 05:53:39PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (address@hidden) wrote:
> > When accept failed, we should setup errp with the reason. More
> > importantly, the caller may assume errp be non-NULL when error happens,
> > and not setting the errp may crash QEMU.
> > 
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  io/channel-socket.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > index 53386b7..7bc308e 100644
> > --- a/io/channel-socket.c
> > +++ b/io/channel-socket.c
> > @@ -344,6 +344,7 @@ qio_channel_socket_accept(QIOChannelSocket *ioc,
> >          if (errno == EINTR) {
> >              goto retry;
> >          }
> > +        error_setg_errno(errp, errno, "Unable to accept connection");
> >          goto error;
> 
> OK, but this code actually has a bigger problem as well:
> 
> the original is:
> 
>     cioc->fd = qemu_accept(ioc->fd, (struct sockaddr *)&cioc->remoteAddr,
>                            &cioc->remoteAddrLen);
>     if (cioc->fd < 0) {
>         trace_qio_channel_socket_accept_fail(ioc);
>         if (errno == EINTR) {
>             goto retry;
>         }
>         goto error;
>     }
> 
> Stefan confirmed that trace_ doesn't preserve errno; so the if
> following it is wrong.  It needs to preserve errno.

Ah... If so, not sure whether we can do the reservation in trace codes
in general?

For this one, I can just move the trace_*() below the errno check.
After all, if EINTR is got, it's not really a fail, so imho we should
not trace it with "accept fail".

> 
> (Again this patch can go on it's own)

Yes. For these patches, I intentionally put them at the beginning of
the series (for easier picking up standalone). Do you (or Juan?) want
me to repost these patches separately?

-- 
Peter Xu