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

[Stefan Hajnoczi]

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?