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 <stefa...@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
