On Mon, Jul 31, 2017 at 05:53:39PM +0100, Dr. David Alan Gilbert wrote: > * Peter Xu (pet...@redhat.com) 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 <pet...@redhat.com> > > --- > > 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