From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51103) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQukQ-0005ai-4B for qemu-devel@nongnu.org; Wed, 03 Feb 2016 05:33:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQukM-0007pL-QI for qemu-devel@nongnu.org; Wed, 03 Feb 2016 05:33:18 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34810) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQukM-0007pE-Ga for qemu-devel@nongnu.org; Wed, 03 Feb 2016 05:33:14 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 250598E748 for ; Wed, 3 Feb 2016 10:33:14 +0000 (UTC) Date: Wed, 3 Feb 2016 10:33:10 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20160203103309.GG2376@work-vm> References: <1452599056-27357-1-git-send-email-berrange@redhat.com> <1452599056-27357-11-git-send-email-berrange@redhat.com> <20160202181902.GC4498@work-vm> <20160203100217.GA30222@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160203100217.GA30222@redhat.com> Subject: Re: [Qemu-devel] [PATCH v1 10/22] migration: convert tcp socket protocol to use QIOChannel List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: Amit Shah , qemu-devel@nongnu.org, Juan Quintela * Daniel P. Berrange (berrange@redhat.com) wrote: > On Tue, Feb 02, 2016 at 06:19:03PM +0000, Dr. David Alan Gilbert wrote: > > * Daniel P. Berrange (berrange@redhat.com) wrote: > > > Convert the tcp socket migration protocol driver to use > > > QIOChannel and QEMUFileChannel, instead of plain sockets > > > APIs. > > > > > > While this now looks pretty similar to the migration/unix.c > > > file from the previous patch, it was decided not to merge > > > the two, because when TLS is added to the TCP impl later, > > > this file diverge from unix.c once again. > > > > Hmm OK, although I'd kind of like to see merging, but lets > > see the TLS code later in the series.... > > > > > Signed-off-by: Daniel P. Berrange > > > --- > > > migration/tcp.c | 119 ++++++++++++++++++++++++++++++++++++++------------------ > > > 1 file changed, 82 insertions(+), 37 deletions(-) > > > > > > diff --git a/migration/tcp.c b/migration/tcp.c > > > index ae89172..ac73977 100644 > > > --- a/migration/tcp.c > > > +++ b/migration/tcp.c > > > @@ -2,9 +2,11 @@ > > > * QEMU live migration > > > * > > > * Copyright IBM, Corp. 2008 > > > + * Copyright Red Hat, Inc. 2015 > > > * > > > * Authors: > > > * Anthony Liguori > > > + * Daniel P. Berrange > > > * > > > * This work is licensed under the terms of the GNU GPL, version 2. See > > > * the COPYING file in the top-level directory. > > > @@ -17,11 +19,9 @@ > > > > > > #include "qemu-common.h" > > > #include "qemu/error-report.h" > > > -#include "qemu/sockets.h" > > > #include "migration/migration.h" > > > #include "migration/qemu-file.h" > > > -#include "block/block.h" > > > -#include "qemu/main-loop.h" > > > +#include "io/channel-socket.h" > > > > > > //#define DEBUG_MIGRATION_TCP > > > > > > @@ -33,71 +33,116 @@ > > > do { } while (0) > > > #endif > > > > > > -static void tcp_wait_for_connect(int fd, Error *err, void *opaque) > > > + > > > +static SocketAddress *tcp_build_address(const char *host_port, Error **errp) > > > +{ > > > + InetSocketAddress *iaddr = inet_parse(host_port, errp); > > > + SocketAddress *saddr; > > > + > > > + if (!iaddr) { > > > + return NULL; > > > + } > > > + > > > + saddr = g_new0(SocketAddress, 1); > > > + saddr->type = SOCKET_ADDRESS_KIND_INET; > > > + saddr->u.inet = iaddr; > > > + > > > + return saddr; > > > +} > > > + > > > + > > > +static void tcp_outgoing_migration(Object *src, > > > + Error *err, > > > + gpointer opaque) > > > { > > > MigrationState *s = opaque; > > > + QIOChannel *sioc = QIO_CHANNEL(src); > > > > > > - if (fd < 0) { > > > + if (err) { > > > DPRINTF("migrate connect error: %s\n", error_get_pretty(err)); > > > s->file = NULL; > > > migrate_fd_error(s); > > > } else { > > > DPRINTF("migrate connect success\n"); > > > - s->file = qemu_fopen_socket(fd, "wb"); > > > + s->file = qemu_fopen_channel_output(sioc); > > > migrate_fd_connect(s); > > > } > > > + object_unref(src); > > > } > > > > > > -void tcp_start_outgoing_migration(MigrationState *s, const char *host_port, Error **errp) > > > + > > > +void tcp_start_outgoing_migration(MigrationState *s, > > > + const char *host_port, > > > + Error **errp) > > > { > > > - inet_nonblocking_connect(host_port, tcp_wait_for_connect, s, errp); > > > + SocketAddress *saddr = tcp_build_address(host_port, errp); > > > + QIOChannelSocket *sioc; > > > + > > > + if (!saddr) { > > > + return; > > > + } > > > + > > > + sioc = qio_channel_socket_new(); > > > + qio_channel_socket_connect_async(sioc, > > > + saddr, > > > + tcp_outgoing_migration, > > > + s, > > > + NULL); > > > + qapi_free_SocketAddress(saddr); > > > } > > > > > > -static void tcp_accept_incoming_migration(void *opaque) > > > + > > > +static gboolean tcp_accept_incoming_migration(QIOChannel *ioc, > > > + GIOCondition condition, > > > + gpointer opaque) > > > { > > > - struct sockaddr_in addr; > > > - socklen_t addrlen = sizeof(addr); > > > - int s = (intptr_t)opaque; > > > QEMUFile *f; > > > - int c, err; > > > + QIOChannelSocket *cioc; > > > + Error *err = NULL; > > > > > > - do { > > > - c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen); > > > - err = socket_error(); > > > - } while (c < 0 && err == EINTR); > > > - qemu_set_fd_handler(s, NULL, NULL, NULL); > > > - closesocket(s); > > > - > > > - DPRINTF("accepted migration\n"); > > > - > > > - if (c < 0) { > > > + cioc = qio_channel_socket_accept(QIO_CHANNEL_SOCKET(ioc), > > > + &err); > > > + if (!cioc) { > > > error_report("could not accept migration connection (%s)", > > > - strerror(err)); > > > - return; > > > - } > > > - > > > - f = qemu_fopen_socket(c, "rb"); > > > - if (f == NULL) { > > > - error_report("could not qemu_fopen socket"); > > > + error_get_pretty(err)); > > > goto out; > > > } > > > > > > + DPRINTF("accepted migration\n"); > > > + > > > + f = qemu_fopen_channel_input(QIO_CHANNEL(cioc)); > > > + object_unref(OBJECT(cioc)); > > > + > > > process_incoming_migration(f); > > > - return; > > > > > > out: > > > - closesocket(c); > > > + /* Close listening socket as its no longer needed */ > > > + qio_channel_close(ioc, NULL); > > > + return FALSE; > > > } > > > > > > + > > > void tcp_start_incoming_migration(const char *host_port, Error **errp) > > > { > > > - int s; > > > + SocketAddress *saddr = tcp_build_address(host_port, errp); > > > + QIOChannelSocket *listen_ioc; > > > > > > - s = inet_listen(host_port, NULL, 256, SOCK_STREAM, 0, errp); > > > - if (s < 0) { > > > + if (!saddr) { > > > return; > > > } > > > > > > - qemu_set_fd_handler(s, tcp_accept_incoming_migration, NULL, > > > - (void *)(intptr_t)s); > > > + listen_ioc = qio_channel_socket_new(); > > > + if (qio_channel_socket_listen_sync(listen_ioc, saddr, errp) < 0) { > > > > In this case, although weird, that could block couldn't it? > > The case I'm thinking of is using the migrate_incoming command with a hostname > > (I've used it with hostnames before as aliases for networks to listen on, > > admittedly it be rare for it to be DNS that would block). > > Yes it certainly could block and this is undesirable in general. The problem > is that the only way we can provide errors back for the monitor reponse is > if we do this synchronously from this method. If I ran async then we'd have > no ability to report errors about non-existant host names, already in use tcp > port, etc. > > I'm not really sure what todo about this long term, but the only way would > be if the monitor response was able to be sent back asychronously from > this method call. Then we can start an async listen call, and respond to > the monitor when done. > > Since the current code being replaced is also blocking, I felt solving > that problem is best left to a future patch series. At least with this > explicit method naming with _sync() suffix we can easily identify the > problems in future Yeh that's fair enough. Failed incoming migrations generally just quit anyway. Dave > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK