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 (berra...@redhat.com) 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 (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 
> > > > > ---
> > > > >  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 
> > > > *)>remoteAddr,
> > > >>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

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

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berra...@redhat.com) 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 (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 
> > > > ---
> > > >  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 *)>remoteAddr,
> > >>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.

Dave

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

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

[Daniel P. Berrange]

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 (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 
> > > ---
> > >  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 *)>remoteAddr,
> >>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.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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 (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 
> > ---
> >  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 *)>remoteAddr,
>>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

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

[Dr. David Alan Gilbert]

* 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 
> ---
>  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 *)>remoteAddr,
   >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.

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

Dave

>  }
>  
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

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

[Peter Xu]

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 
---
 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;
 }
 
-- 
2.7.4