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)
>> +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
