On Tue, Feb 02, 2016 at 06:19:03PM +0000, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berra...@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 <berra...@redhat.com>
> > ---
> > 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 <aligu...@us.ibm.com>
> > + * Daniel P. Berrange <berra...@redhat.com>
> > *
> > * 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
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 :|
