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

[Peter Xu]

On Tue, Aug 01, 2017 at 09:55:08AM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (address@hidden) wrote:
> > On Tue, Aug 01, 2017 at 10:25:19AM +0800, Peter Xu wrote:
> > > 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".
> > 
> > Agreed, we just need to move the trace below the if.
> 
> Peter: Can you split this as a separate patch and it seems OK to try and
> put this in 2.10 since it's a strict bug fix.

Sure!  Then I'll possibly include the comment fix patch as well.

-- 
Peter Xu