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. > +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. > +static gushort socket_establish_writable(void *opaque) > +{ > + NetSocketState *s = opaque; > + > + if (s->write_poll) { > + return G_IO_OUT; Errors/hang up? > @@ -440,9 +529,20 @@ static NetSocketState > *net_socket_fd_init_stream(NetClientState *peer, > s->listen_fd = -1; > > if (is_connected) { > - net_socket_connect(s); > + assert(!s->nsrc); > + s->nsrc = event_source_new(fd, net_socket_establish_handler, s); > + s->nsrc->readable = socket_establish_readable; > + s->nsrc->writable = socket_establish_writable; > + nc->info->bind_ctx(nc, NULL); > + net_socket_read_poll(s, true); > + net_socket_write_poll(s, true); > } else { > - qemu_set_fd_handler(s->fd, NULL, net_socket_connect, s); > + assert(!s->nsrc); > + s->nsrc = event_source_new(fd, net_socket_connect_handler, s); > + s->nsrc->readable = socket_connecting_readable; The original code wants writeable, not readable. > +static gboolean net_socket_listen_handler(gpointer data) > +{ > + EventGSource *nsrc = data; > + NetSocketState *s = nsrc->opaque; > struct sockaddr_in saddr; > socklen_t len; > int fd; > > - for(;;) { > - len = sizeof(saddr); > - fd = qemu_accept(s->listen_fd, (struct sockaddr *)&saddr, &len); > - if (fd < 0 && errno != EINTR) { > - return; > - } else if (fd >= 0) { > - qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL); > - break; > - } > + len = sizeof(saddr); > + fd = qemu_accept(s->listen_fd, (struct sockaddr *)&saddr, &len); > + if (fd < 0 && errno != EINTR) { > + return false; > } This breaks the code when accept(2) is interrupted by a signal and we get -1, errno == EINTR. Why did you remove the loop?