[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/4] net/socket: Convert the non-blocking con
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/4] net/socket: Convert the non-blocking connection mechanism to QIOchannel |
Date: |
Thu, 27 Apr 2017 17:20:55 +0100 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
On Wed, Apr 26, 2017 at 04:04:15PM +0800, Mao Zhongyi wrote:
> 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;
> + 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);
> goto end;
> }
>
> @@ -567,7 +574,7 @@ static int net_socket_connect_init(NetClientState *peer,
> const char *host_str)
> {
> socket_connect_data *c = g_new0(socket_connect_data, 1);
> - int fd = -1;
> + QIOChannelSocket *sioc;
> Error *local_error = NULL;
>
> c->peer = peer;
> @@ -578,11 +585,12 @@ static int net_socket_connect_init(NetClientState *peer,
> goto err;
> }
>
> - fd = socket_connect(c->saddr, net_socket_connected, c, &local_error);
> - if (fd < 0) {
> - goto err;
> - }
> -
> + sioc = qio_channel_socket_new();
> + qio_channel_socket_connect_async(sioc,
> + c->saddr,
> + net_socket_connected,
> + c,
> + NULL);
> return 0;
You've not saved a copy of the QIOChannelSocket poiinter here, and
the net_socket_connected() method doesn't release the reference
either. So htere is a memory leak. Of course if you relesae the
reference in net_socket_connected(), then you need to dup() the
file descriptor you're borrowing.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|