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

[liu ping fan]

On Fri, Apr 26, 2013 at 5:48 PM, Stefan Hajnoczi <address@hidden> 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)
>> +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?

>> @@ -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.
>
Will fix it.
>> +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?
Oh, will fix it.

Thanks,
Pingfan