Re: [Qemu-devel] [RFC PATCH v5 05/14] net: port socket to GSource

[Stefan Hajnoczi <stefanha@redhat.com>]

On Sat, Apr 27, 2013 at 03:09:10PM +0800, liu ping fan wrote:
> On Fri, Apr 26, 2013 at 5:48 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Fri, Apr 26, 2013 at 10:47:26AM +0800, Liu Ping Fan wrote:
> >> @@ -141,6 +134,59 @@ static ssize_t net_socket_receive_dgram(NetClientState *nc, const uint8_t *buf,
> >>      return ret;
> >>  }
> >>
> >> +static gushort socket_connecting_readable(void *opaque)
> >> +{
> >> +    return G_IO_IN;
> >> +}
> >> +
> >> +static gushort socket_listen_readable(void *opaque)
> >> +{
> >> +    /* listen only handle in-req, no err */
> >> +    return G_IO_IN;
> >
> > From the accept(2) man page:
> >
> > "Linux accept() (and accept4()) passes already-pending network errors on
> > the new socket as an error code from accept()."
> >
> > So we must handle errors from accept(2), please use G_IO_IN | G_IO_HUP |
> > G_IO_ERR.
> >
> Here, we handle listen(2), not accept(2)

Look again, the handler invokes accept(2).  listen(2) was called to put
the socket into the listening state but now we are monitoring for
accept.

> >> +static gushort socket_establish_readable(void *opaque)
> >> +{
> >> +    NetSocketState *s = opaque;
> >> +
> >> +    /* rely on net_socket_send to handle err */
> >> +    if (s->read_poll && net_socket_can_send(s)) {
> >> +        return G_IO_IN|G_IO_HUP|G_IO_ERR;
> >> +    }
> >> +    return G_IO_HUP|G_IO_ERR;
> >> +}
> >
> > This new function always monitors G_IO_HUP | G_IO_ERR.  The old code
> > only monitored it when read_poll == true and net_socket_can_send() ==
> > true.
> >
> > Please preserve semantics.
> >
> But the only the code in net_socket_send() will handle the err
> condition. See the code behind "/* end of connection */".  And I think
> it is safely to handle err, even when the peer is not ready to
> receive.
> 
> >> +static gushort socket_establish_writable(void *opaque)
> >> +{
> >> +    NetSocketState *s = opaque;
> >> +
> >> +    if (s->write_poll) {
> >> +        return G_IO_OUT;
> >
> > Errors/hang up?
> >
> As explained above, net_socket_writable() does not handle the err
> condition. But maybe we need the qemu_flush_queued_packets() in it?

net_socket_receive() does handle send(2) errors but it does so
differently from net_socket_send().  It fails the packet and resets
->send_index.

The change you made doesn't really solve this because an error could
still happen right when QEMU calls send(2) and therefore not be
processed by net_socket_send().

Changing the send/receive error handling is something that could be done
carefully in a separate patch.  But please preserve semantics in
conversion patches - it makes them easy to review and merge.

As explained above, I'm not convinced that the change you made is
useful.

Stefan