Re: [Qemu-devel] [PATCH v2 1/4] net/socket: Convert the non-blocking connection mechanism to QIOchannel

[Markus Armbruster]

Markus Armbruster <address@hidden> writes:

> Mao Zhongyi <address@hidden> writes:
>
>> Currently, socket connection in net is realized by an old
>> mechanism which is non-blocking.
>>
>> That old mechanism may cause net blocks on DNS lookups and
>> QEmu has already replaced it with QIOchannel in many features,
>> such as migration.
>>
>> Convert it to QIOchannel for net as well.
>>
>> CC: address@hidden, address@hidden, address@hidden
>> Signed-off-by: Mao Zhongyi <address@hidden>
>> ---
>> The test steps like this:
>>
>>     $ qemu-system-x86_64 -net nic -net socket,listen=:1234 ~/img/test.img
>>     $ qemu-system-x86_64 -net nic -net socket,connect=127.0.0.1:1234 
>> ~/img/test.img
>>
>> No exception.
>>
>>  net/socket.c | 30 +++++++++++++++++++-----------
>>  1 file changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/net/socket.c b/net/socket.c
>> index b8c931e..52f9dce 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -33,6 +33,7 @@
>>  #include "qemu/sockets.h"
>>  #include "qemu/iov.h"
>>  #include "qemu/main-loop.h"
>> +#include "io/channel-socket.h"
>>  
>>  typedef struct NetSocketState {
>>      NetClientState nc;
>> @@ -525,16 +526,22 @@ typedef struct {
>>      char *name;
>>  } socket_connect_data;
>>  
>> -static void socket_connect_data_free(socket_connect_data *c)
>> +static void socket_connect_data_free(void *opaque)
>>  {
>> +    socket_connect_data *c = opaque;
>
> Blank line between declarations and statements, please.
>
>> +    if (!c) {
>> +        return;
>> +    }
>> +
>>      qapi_free_SocketAddress(c->saddr);
>>      g_free(c->model);
>>      g_free(c->name);
>>      g_free(c);
>>  }
>>  
>> -static void net_socket_connected(int fd, Error *err, void *opaque)
>> +static void net_socket_connected(QIOTask *task, void *opaque)
>>  {
>> +    QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
>>      socket_connect_data *c = opaque;
>>      NetSocketState *s;
>>      char *addr_str = NULL;
>> @@ -543,13 +550,13 @@ static void net_socket_connected(int fd, Error *err, 
>> void *opaque)
>>      addr_str = socket_address_to_string(c->saddr, &local_error);
>>      if (addr_str == NULL) {
>>          error_report_err(local_error);
>> -        closesocket(fd);
>> +        closesocket(sioc->fd);
>>          goto end;
>>      }
>>  
>> -    s = net_socket_fd_init(c->peer, c->model, c->name, fd, true);
>> +    s = net_socket_fd_init(c->peer, c->model, c->name, sioc->fd, true);
>>      if (!s) {
>> -        closesocket(fd);
>> +        closesocket(sioc->fd);

Actually, net_socket_fd_init() closes sioc->fd when it fails.  Closing
it again in the caller is wrong.  None of the other callers does it.
Please drop this line in a separate patch, before this one.

>>          goto end;
>>      }
>>  
[...]