[libvirt] [PATCH 0/6] Use fd: protocol for migration
Since qemu doesn't give us any reasonable errors when migration fails because of connection issues, we now create a connection to destination qemu ourselves and just pass the created socket to qemu. Daniel P. Berrange (1): Add API for duplicating a socket/client file descriptor Jiri Denemark (5): Add backlog parameter to virNetSocketListen Support changing UNIX socket owner in virNetSocketNewListenUNIX qemu: Refactor do{Tunnel,Native}Migrate functions qemu: Use virNetSocket for tunneled migration qemu: Use fd: protocol for migration src/qemu/qemu_migration.c | 541 +++++++++++++++++++++-------------------- src/rpc/virnetclient.c | 20 ++ src/rpc/virnetclient.h | 3 + src/rpc/virnetserverservice.c | 6 +- src/rpc/virnetsocket.c | 29 ++- src/rpc/virnetsocket.h | 5 +- tests/virnetsockettest.c | 10 +- 7 files changed, 336 insertions(+), 278 deletions(-) -- 1.7.6
From: "Daniel P. Berrange" <berrange@redhat.com> * src/rpc/virnetsocket.c, src/rpc/virnetsocket.h: Add virNetSocketDupFD() * src/rpc/virnetclient.c, src/rpc/virnetclient.h: Add virNetClientDupFD() and virNetClientGetFD() --- src/rpc/virnetclient.c | 20 ++++++++++++++++++++ src/rpc/virnetclient.h | 3 +++ src/rpc/virnetsocket.c | 18 ++++++++++++++++++ src/rpc/virnetsocket.h | 2 ++ 4 files changed, 43 insertions(+), 0 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index b845555..31d79ef 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -233,6 +233,26 @@ void virNetClientRef(virNetClientPtr client) } +int virNetClientGetFD(virNetClientPtr client) +{ + int fd; + virNetClientLock(client); + fd = virNetSocketGetFD(client->sock); + virNetClientUnlock(client); + return fd; +} + + +int virNetClientDupFD(virNetClientPtr client, bool cloexec) +{ + int fd; + virNetClientLock(client); + fd = virNetSocketDupFD(client->sock, cloexec); + virNetClientUnlock(client); + return fd; +} + + void virNetClientFree(virNetClientPtr client) { int i; diff --git a/src/rpc/virnetclient.h b/src/rpc/virnetclient.h index 90d19d3..1fabcfd 100644 --- a/src/rpc/virnetclient.h +++ b/src/rpc/virnetclient.h @@ -53,6 +53,9 @@ virNetClientPtr virNetClientNewExternal(const char **cmdargv); void virNetClientRef(virNetClientPtr client); +int virNetClientGetFD(virNetClientPtr client); +int virNetClientDupFD(virNetClientPtr client, bool cloexec); + int virNetClientAddProgram(virNetClientPtr client, virNetClientProgramPtr prog); diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 992e33a..c222743 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -28,6 +28,7 @@ #include <unistd.h> #include <sys/wait.h> #include <signal.h> +#include <fcntl.h> #ifdef HAVE_NETINET_TCP_H # include <netinet/tcp.h> @@ -710,6 +711,23 @@ int virNetSocketGetFD(virNetSocketPtr sock) } +int virNetSocketDupFD(virNetSocketPtr sock, bool cloexec) +{ + int fd; + + if (cloexec) + fd = fcntl(sock->fd, F_DUPFD_CLOEXEC, (long) 0); + else + fd = dup(sock->fd); + if (fd < 0) { + virReportSystemError(errno, "%s", + _("Unable to copy socket file handle")); + return -1; + } + return fd; +} + + bool virNetSocketIsLocal(virNetSocketPtr sock) { bool isLocal = false; diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index 1e1c63c..d6c85d2 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -77,6 +77,8 @@ int virNetSocketNewConnectExternal(const char **cmdargv, virNetSocketPtr *addr); int virNetSocketGetFD(virNetSocketPtr sock); +int virNetSocketDupFD(virNetSocketPtr sock, bool cloexec); + bool virNetSocketIsLocal(virNetSocketPtr sock); int virNetSocketGetPort(virNetSocketPtr sock); -- 1.7.6
On Mon, Aug 15, 2011 at 09:58:11AM +0200, Jiri Denemark wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
* src/rpc/virnetsocket.c, src/rpc/virnetsocket.h: Add virNetSocketDupFD() * src/rpc/virnetclient.c, src/rpc/virnetclient.h: Add virNetClientDupFD() and virNetClientGetFD() --- src/rpc/virnetclient.c | 20 ++++++++++++++++++++ src/rpc/virnetclient.h | 3 +++ src/rpc/virnetsocket.c | 18 ++++++++++++++++++ src/rpc/virnetsocket.h | 2 ++ 4 files changed, 43 insertions(+), 0 deletions(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index b845555..31d79ef 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -233,6 +233,26 @@ void virNetClientRef(virNetClientPtr client) }
+int virNetClientGetFD(virNetClientPtr client) +{ + int fd; + virNetClientLock(client); + fd = virNetSocketGetFD(client->sock); + virNetClientUnlock(client); + return fd; +} + + +int virNetClientDupFD(virNetClientPtr client, bool cloexec) +{ + int fd; + virNetClientLock(client); + fd = virNetSocketDupFD(client->sock, cloexec); + virNetClientUnlock(client); + return fd; +} + + void virNetClientFree(virNetClientPtr client) { int i; diff --git a/src/rpc/virnetclient.h b/src/rpc/virnetclient.h index 90d19d3..1fabcfd 100644 --- a/src/rpc/virnetclient.h +++ b/src/rpc/virnetclient.h @@ -53,6 +53,9 @@ virNetClientPtr virNetClientNewExternal(const char **cmdargv);
void virNetClientRef(virNetClientPtr client);
+int virNetClientGetFD(virNetClientPtr client); +int virNetClientDupFD(virNetClientPtr client, bool cloexec); + int virNetClientAddProgram(virNetClientPtr client, virNetClientProgramPtr prog);
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 992e33a..c222743 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -28,6 +28,7 @@ #include <unistd.h> #include <sys/wait.h> #include <signal.h> +#include <fcntl.h>
#ifdef HAVE_NETINET_TCP_H # include <netinet/tcp.h> @@ -710,6 +711,23 @@ int virNetSocketGetFD(virNetSocketPtr sock) }
+int virNetSocketDupFD(virNetSocketPtr sock, bool cloexec) +{ + int fd; + + if (cloexec) + fd = fcntl(sock->fd, F_DUPFD_CLOEXEC, (long) 0); + else + fd = dup(sock->fd); + if (fd < 0) { + virReportSystemError(errno, "%s", + _("Unable to copy socket file handle")); + return -1; + } + return fd; +} + + bool virNetSocketIsLocal(virNetSocketPtr sock) { bool isLocal = false; diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index 1e1c63c..d6c85d2 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -77,6 +77,8 @@ int virNetSocketNewConnectExternal(const char **cmdargv, virNetSocketPtr *addr);
int virNetSocketGetFD(virNetSocketPtr sock); +int virNetSocketDupFD(virNetSocketPtr sock, bool cloexec); + bool virNetSocketIsLocal(virNetSocketPtr sock);
int virNetSocketGetPort(virNetSocketPtr sock);
looks fine ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, Aug 15, 2011 at 09:58:11 +0200, Jiri Denemark wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
* src/rpc/virnetsocket.c, src/rpc/virnetsocket.h: Add virNetSocketDupFD() * src/rpc/virnetclient.c, src/rpc/virnetclient.h: Add virNetClientDupFD() and virNetClientGetFD() ---
Ah, I forgot to mention that this patch is in fact a v2 of https://www.redhat.com/archives/libvir-list/2011-July/msg00340.html sent as part of "Add a virtlockd lock manager daemon" series. I modified it according to Eric's suggestions. Jirka
On 08/15/2011 03:43 AM, Jiri Denemark wrote:
On Mon, Aug 15, 2011 at 09:58:11 +0200, Jiri Denemark wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
* src/rpc/virnetsocket.c, src/rpc/virnetsocket.h: Add virNetSocketDupFD() * src/rpc/virnetclient.c, src/rpc/virnetclient.h: Add virNetClientDupFD() and virNetClientGetFD() ---
Ah, I forgot to mention that this patch is in fact a v2 of https://www.redhat.com/archives/libvir-list/2011-July/msg00340.html sent as part of "Add a virtlockd lock manager daemon" series. I modified it according to Eric's suggestions.
Except you missed one change I had pointed out there: bootstrap.conf needs to list 'fcntl' in the list of gnulib modules (without it, gnulib doesn't guarantee that F_DUPFD_CLOEXEC will work on mingw). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Aug 15, 2011 at 06:49:58 -0600, Eric Blake wrote:
On 08/15/2011 03:43 AM, Jiri Denemark wrote:
On Mon, Aug 15, 2011 at 09:58:11 +0200, Jiri Denemark wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
* src/rpc/virnetsocket.c, src/rpc/virnetsocket.h: Add virNetSocketDupFD() * src/rpc/virnetclient.c, src/rpc/virnetclient.h: Add virNetClientDupFD() and virNetClientGetFD() ---
Ah, I forgot to mention that this patch is in fact a v2 of https://www.redhat.com/archives/libvir-list/2011-July/msg00340.html sent as part of "Add a virtlockd lock manager daemon" series. I modified it according to Eric's suggestions.
Except you missed one change I had pointed out there: bootstrap.conf needs to list 'fcntl' in the list of gnulib modules (without it, gnulib doesn't guarantee that F_DUPFD_CLOEXEC will work on mingw).
Hmm, I saw fcntl-h in bootstrap.conf, is that something else? Jirka
On 08/15/2011 06:57 AM, Jiri Denemark wrote:
Except you missed one change I had pointed out there: bootstrap.conf needs to list 'fcntl' in the list of gnulib modules (without it, gnulib doesn't guarantee that F_DUPFD_CLOEXEC will work on mingw).
Hmm, I saw fcntl-h in bootstrap.conf, is that something else?
fcntl-h: provides replacement <fcntl.h>, which guarantees definitions for several useful O_* constants. fcntl: depends on fcntl-h, and additionally provides replacement fcntl(), F_*, and FD_* constants that can be portably implemented (not all F_* flags can be ported to mingw - F_GETFL being a notable omission, unfortunately). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/15/2011 01:58 AM, Jiri Denemark wrote:
@@ -710,6 +711,23 @@ int virNetSocketGetFD(virNetSocketPtr sock) }
+int virNetSocketDupFD(virNetSocketPtr sock, bool cloexec) +{ + int fd; + + if (cloexec) + fd = fcntl(sock->fd, F_DUPFD_CLOEXEC, (long) 0);
The third argument is unneeded, and skipping it will avoid a stupid-looking cast. But leaving it doesn't hurt either, if you already pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Aug 15, 2011 at 06:42:15 -0600, Eric Blake wrote:
On 08/15/2011 01:58 AM, Jiri Denemark wrote:
@@ -710,6 +711,23 @@ int virNetSocketGetFD(virNetSocketPtr sock) }
+int virNetSocketDupFD(virNetSocketPtr sock, bool cloexec) +{ + int fd; + + if (cloexec) + fd = fcntl(sock->fd, F_DUPFD_CLOEXEC, (long) 0);
The third argument is unneeded, and skipping it will avoid a stupid-looking cast. But leaving it doesn't hurt either, if you already pushed.
Ah, nice, fcntl man page didn't mention this possibility. I was just about to push but I'll remove the argument first. Jirka
So that callers can change the default value. --- src/rpc/virnetserverservice.c | 4 ++-- src/rpc/virnetsocket.c | 4 ++-- src/rpc/virnetsocket.h | 2 +- tests/virnetsockettest.c | 6 +++--- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 8c9ed1e..e63603f 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -127,7 +127,7 @@ virNetServerServicePtr virNetServerServiceNewTCP(const char *nodename, goto error; for (i = 0 ; i < svc->nsocks ; i++) { - if (virNetSocketListen(svc->socks[i]) < 0) + if (virNetSocketListen(svc->socks[i], 0) < 0) goto error; /* IO callback is initially disabled, until we're ready @@ -187,7 +187,7 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path, goto error; for (i = 0 ; i < svc->nsocks ; i++) { - if (virNetSocketListen(svc->socks[i]) < 0) + if (virNetSocketListen(svc->socks[i], 0) < 0) goto error; /* IO callback is initially disabled, until we're ready diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index c222743..c19dcfa 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -1076,10 +1076,10 @@ ssize_t virNetSocketWrite(virNetSocketPtr sock, const char *buf, size_t len) } -int virNetSocketListen(virNetSocketPtr sock) +int virNetSocketListen(virNetSocketPtr sock, int backlog) { virMutexLock(&sock->lock); - if (listen(sock->fd, 30) < 0) { + if (listen(sock->fd, backlog > 0 ? backlog : 30) < 0) { virReportSystemError(errno, "%s", _("Unable to listen on socket")); virMutexUnlock(&sock->lock); return -1; diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index d6c85d2..24110a6 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -105,7 +105,7 @@ void virNetSocketFree(virNetSocketPtr sock); const char *virNetSocketLocalAddrString(virNetSocketPtr sock); const char *virNetSocketRemoteAddrString(virNetSocketPtr sock); -int virNetSocketListen(virNetSocketPtr sock); +int virNetSocketListen(virNetSocketPtr sock, int backlog); int virNetSocketAccept(virNetSocketPtr sock, virNetSocketPtr *clientsock); diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index e72b9a0..fba7e15 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -159,7 +159,7 @@ static int testSocketTCPAccept(const void *opaque) goto cleanup; for (i = 0 ; i < nlsock ; i++) { - if (virNetSocketListen(lsock[i]) < 0) + if (virNetSocketListen(lsock[i], 0) < 0) goto cleanup; } @@ -217,7 +217,7 @@ static int testSocketUNIXAccept(const void *data ATTRIBUTE_UNUSED) if (virNetSocketNewListenUNIX(path, 0700, getgid(), &lsock) < 0) goto cleanup; - if (virNetSocketListen(lsock) < 0) + if (virNetSocketListen(lsock, 0) < 0) goto cleanup; if (virNetSocketNewConnectUNIX(path, false, NULL, &csock) < 0) @@ -276,7 +276,7 @@ static int testSocketUNIXAddrs(const void *data ATTRIBUTE_UNUSED) goto cleanup; } - if (virNetSocketListen(lsock) < 0) + if (virNetSocketListen(lsock, 0) < 0) goto cleanup; if (virNetSocketNewConnectUNIX(path, false, NULL, &csock) < 0) -- 1.7.6
On Mon, Aug 15, 2011 at 09:58:12AM +0200, Jiri Denemark wrote:
So that callers can change the default value. --- src/rpc/virnetserverservice.c | 4 ++-- src/rpc/virnetsocket.c | 4 ++-- src/rpc/virnetsocket.h | 2 +- tests/virnetsockettest.c | 6 +++--- 4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 8c9ed1e..e63603f 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -127,7 +127,7 @@ virNetServerServicePtr virNetServerServiceNewTCP(const char *nodename, goto error;
for (i = 0 ; i < svc->nsocks ; i++) { - if (virNetSocketListen(svc->socks[i]) < 0) + if (virNetSocketListen(svc->socks[i], 0) < 0) goto error;
/* IO callback is initially disabled, until we're ready @@ -187,7 +187,7 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path, goto error;
for (i = 0 ; i < svc->nsocks ; i++) { - if (virNetSocketListen(svc->socks[i]) < 0) + if (virNetSocketListen(svc->socks[i], 0) < 0) goto error;
/* IO callback is initially disabled, until we're ready diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index c222743..c19dcfa 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -1076,10 +1076,10 @@ ssize_t virNetSocketWrite(virNetSocketPtr sock, const char *buf, size_t len) }
-int virNetSocketListen(virNetSocketPtr sock) +int virNetSocketListen(virNetSocketPtr sock, int backlog) { virMutexLock(&sock->lock); - if (listen(sock->fd, 30) < 0) { + if (listen(sock->fd, backlog > 0 ? backlog : 30) < 0) {
Okay that's where we override the old default ...
virReportSystemError(errno, "%s", _("Unable to listen on socket")); virMutexUnlock(&sock->lock); return -1; diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index d6c85d2..24110a6 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -105,7 +105,7 @@ void virNetSocketFree(virNetSocketPtr sock); const char *virNetSocketLocalAddrString(virNetSocketPtr sock); const char *virNetSocketRemoteAddrString(virNetSocketPtr sock);
-int virNetSocketListen(virNetSocketPtr sock); +int virNetSocketListen(virNetSocketPtr sock, int backlog); int virNetSocketAccept(virNetSocketPtr sock, virNetSocketPtr *clientsock);
diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index e72b9a0..fba7e15 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -159,7 +159,7 @@ static int testSocketTCPAccept(const void *opaque) goto cleanup;
for (i = 0 ; i < nlsock ; i++) { - if (virNetSocketListen(lsock[i]) < 0) + if (virNetSocketListen(lsock[i], 0) < 0) goto cleanup; }
@@ -217,7 +217,7 @@ static int testSocketUNIXAccept(const void *data ATTRIBUTE_UNUSED) if (virNetSocketNewListenUNIX(path, 0700, getgid(), &lsock) < 0) goto cleanup;
- if (virNetSocketListen(lsock) < 0) + if (virNetSocketListen(lsock, 0) < 0) goto cleanup;
if (virNetSocketNewConnectUNIX(path, false, NULL, &csock) < 0) @@ -276,7 +276,7 @@ static int testSocketUNIXAddrs(const void *data ATTRIBUTE_UNUSED) goto cleanup; }
- if (virNetSocketListen(lsock) < 0) + if (virNetSocketListen(lsock, 0) < 0) goto cleanup;
if (virNetSocketNewConnectUNIX(path, false, NULL, &csock) < 0)
Assuming that the facts it compiles prove that all cases got covered, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
This patch allows owner's UID to be changed as well. --- src/rpc/virnetserverservice.c | 2 +- src/rpc/virnetsocket.c | 7 ++++--- src/rpc/virnetsocket.h | 1 + tests/virnetsockettest.c | 4 ++-- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index e63603f..9f82a8d 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -182,7 +182,7 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path, if (virNetSocketNewListenUNIX(path, mask, - grp, + -1, grp, &svc->socks[0]) < 0) goto error; diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index c19dcfa..23ec5ca 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -302,6 +302,7 @@ error: #if HAVE_SYS_UN_H int virNetSocketNewListenUNIX(const char *path, mode_t mask, + uid_t user, gid_t grp, virNetSocketPtr *retsock) { @@ -344,10 +345,10 @@ int virNetSocketNewListenUNIX(const char *path, /* chown() doesn't work for abstract sockets but we use them only * if libvirtd runs unprivileged */ - if (grp != 0 && chown(path, -1, grp)) { + if (grp != 0 && chown(path, user, grp)) { virReportSystemError(errno, - _("Failed to change group ID of '%s' to %u"), - path, (unsigned int) grp); + _("Failed to change ownership of '%s' to %d:%d"), + path, (int) user, (int) grp); goto error; } diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index 24110a6..f7e5ebb 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -47,6 +47,7 @@ int virNetSocketNewListenTCP(const char *nodename, int virNetSocketNewListenUNIX(const char *path, mode_t mask, + uid_t user, gid_t grp, virNetSocketPtr *addr); diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index fba7e15..fae15a3 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -214,7 +214,7 @@ static int testSocketUNIXAccept(const void *data ATTRIBUTE_UNUSED) } } - if (virNetSocketNewListenUNIX(path, 0700, getgid(), &lsock) < 0) + if (virNetSocketNewListenUNIX(path, 0700, -1, getgid(), &lsock) < 0) goto cleanup; if (virNetSocketListen(lsock, 0) < 0) @@ -263,7 +263,7 @@ static int testSocketUNIXAddrs(const void *data ATTRIBUTE_UNUSED) } } - if (virNetSocketNewListenUNIX(path, 0700, getgid(), &lsock) < 0) + if (virNetSocketNewListenUNIX(path, 0700, -1, getgid(), &lsock) < 0) goto cleanup; if (STRNEQ(virNetSocketLocalAddrString(lsock), "127.0.0.1;0")) { -- 1.7.6
On Mon, Aug 15, 2011 at 09:58:13AM +0200, Jiri Denemark wrote:
This patch allows owner's UID to be changed as well. --- src/rpc/virnetserverservice.c | 2 +- src/rpc/virnetsocket.c | 7 ++++--- src/rpc/virnetsocket.h | 1 + tests/virnetsockettest.c | 4 ++-- 4 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index e63603f..9f82a8d 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -182,7 +182,7 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path,
if (virNetSocketNewListenUNIX(path, mask, - grp, + -1, grp,
Only comment would be that if we started with one line per arg, the patch should probably keep that (but I don't like this much so ...)
&svc->socks[0]) < 0) goto error;
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index c19dcfa..23ec5ca 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -302,6 +302,7 @@ error: #if HAVE_SYS_UN_H int virNetSocketNewListenUNIX(const char *path, mode_t mask, + uid_t user, gid_t grp, virNetSocketPtr *retsock) { @@ -344,10 +345,10 @@ int virNetSocketNewListenUNIX(const char *path, /* chown() doesn't work for abstract sockets but we use them only * if libvirtd runs unprivileged */ - if (grp != 0 && chown(path, -1, grp)) { + if (grp != 0 && chown(path, user, grp)) { virReportSystemError(errno, - _("Failed to change group ID of '%s' to %u"), - path, (unsigned int) grp); + _("Failed to change ownership of '%s' to %d:%d"), + path, (int) user, (int) grp); goto error; }
diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index 24110a6..f7e5ebb 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -47,6 +47,7 @@ int virNetSocketNewListenTCP(const char *nodename,
int virNetSocketNewListenUNIX(const char *path, mode_t mask, + uid_t user, gid_t grp, virNetSocketPtr *addr);
diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index fba7e15..fae15a3 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -214,7 +214,7 @@ static int testSocketUNIXAccept(const void *data ATTRIBUTE_UNUSED) } }
- if (virNetSocketNewListenUNIX(path, 0700, getgid(), &lsock) < 0) + if (virNetSocketNewListenUNIX(path, 0700, -1, getgid(), &lsock) < 0) goto cleanup;
if (virNetSocketListen(lsock, 0) < 0) @@ -263,7 +263,7 @@ static int testSocketUNIXAddrs(const void *data ATTRIBUTE_UNUSED) } }
- if (virNetSocketNewListenUNIX(path, 0700, getgid(), &lsock) < 0) + if (virNetSocketNewListenUNIX(path, 0700, -1, getgid(), &lsock) < 0) goto cleanup;
if (STRNEQ(virNetSocketLocalAddrString(lsock), "127.0.0.1;0")) {
ACK, that too seems uncontroversial Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, Aug 15, 2011 at 17:38:47 +0800, Daniel Veillard wrote:
On Mon, Aug 15, 2011 at 09:58:13AM +0200, Jiri Denemark wrote:
This patch allows owner's UID to be changed as well. --- src/rpc/virnetserverservice.c | 2 +- src/rpc/virnetsocket.c | 7 ++++--- src/rpc/virnetsocket.h | 1 + tests/virnetsockettest.c | 4 ++-- 4 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index e63603f..9f82a8d 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -182,7 +182,7 @@ virNetServerServicePtr virNetServerServiceNewUNIX(const char *path,
if (virNetSocketNewListenUNIX(path, mask, - grp, + -1, grp,
Only comment would be that if we started with one line per arg, the patch should probably keep that (but I don't like this much so ...)
Yeah, I was thinking about uid and gid to be so closely related that it made sense to put them on a single line. I separated them for consistency. Jirka
The core of these two functions is very similar and most of it is even exactly the same. Factor out the core functionality into a separate function to remove code duplication and make further changes easier. --- src/qemu/qemu_migration.c | 499 ++++++++++++++++++++++----------------------- 1 files changed, 239 insertions(+), 260 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4d0e062..1cabbe0 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1265,122 +1265,37 @@ cleanup: } -/* Perform migration using QEMU's native TCP migrate support, - * not encrypted obviously - */ -static int doNativeMigrate(struct qemud_driver *driver, - virDomainObjPtr vm, - const char *uri, - const char *cookiein, - int cookieinlen, - char **cookieout, - int *cookieoutlen, - unsigned int flags, - const char *dname ATTRIBUTE_UNUSED, - unsigned long resource) -{ - int ret = -1; - xmlURIPtr uribits = NULL; - qemuDomainObjPrivatePtr priv = vm->privateData; - unsigned int background_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; - qemuMigrationCookiePtr mig = NULL; - VIR_DEBUG("driver=%p, vm=%p, uri=%s, cookiein=%s, cookieinlen=%d, " - "cookieout=%p, cookieoutlen=%p, flags=%x, dname=%s, resource=%lu", - driver, vm, uri, NULLSTR(cookiein), cookieinlen, - cookieout, cookieoutlen, flags, NULLSTR(dname), resource); - - if (virLockManagerPluginUsesState(driver->lockManager) && - !cookieout) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("Migration with lock driver %s requires cookie support"), - virLockManagerPluginGetName(driver->lockManager)); - return -1; - } - - if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen, - QEMU_MIGRATION_COOKIE_GRAPHICS))) - goto cleanup; - - if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig) < 0) - VIR_WARN("unable to provide data for graphics client relocation"); - - /* Issue the migrate command. */ - if (STRPREFIX(uri, "tcp:") && !STRPREFIX(uri, "tcp://")) { - /* HACK: source host generates bogus URIs, so fix them up */ - char *tmpuri; - if (virAsprintf(&tmpuri, "tcp://%s", uri + strlen("tcp:")) < 0) { - virReportOOMError(); - goto cleanup; - } - uribits = xmlParseURI(tmpuri); - VIR_FREE(tmpuri); - } else { - uribits = xmlParseURI(uri); - } - if (!uribits) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse URI %s"), uri); - goto cleanup; - } - - /* Before EnterMonitor, since qemuProcessStopCPUs already does that */ - if (!(flags & VIR_MIGRATE_LIVE) && - virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - if (qemuMigrationSetOffline(driver, vm) < 0) - goto cleanup; - } - - if (qemuDomainObjEnterMonitorAsync(driver, vm, - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) - goto cleanup; - - if (resource > 0 && - qemuMonitorSetMigrationSpeed(priv->mon, resource) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto cleanup; - } - - if (flags & VIR_MIGRATE_NON_SHARED_DISK) - background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_DISK; - - if (flags & VIR_MIGRATE_NON_SHARED_INC) - background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_INC; - - if (qemuMonitorMigrateToHost(priv->mon, background_flags, uribits->server, - uribits->port) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto cleanup; - } - qemuDomainObjExitMonitorWithDriver(driver, vm); - - if (qemuMigrationWaitForCompletion(driver, vm, - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) - goto cleanup; - - /* When migration completed, QEMU will have paused the - * CPUs for us, but unless we're using the JSON monitor - * we won't have been notified of this, so might still - * think we're running. For v2 protocol this doesn't - * matter because we'll kill the VM soon, but for v3 - * this is important because we stay paused until the - * confirm3 step, but need to release the lock state - */ - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - if (qemuMigrationSetOffline(driver, vm) < 0) - goto cleanup; - } - - if (qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen, 0) < 0) - VIR_WARN("Unable to encode migration cookie"); - - ret = 0; +enum qemuMigrationDestinationType { + MIGRATION_DEST_HOST, + MIGRATION_DEST_UNIX, +}; -cleanup: - qemuMigrationCookieFree(mig); - xmlFreeURI(uribits); - return ret; -} +enum qemuMigrationForwardType { + MIGRATION_FWD_DIRECT, + MIGRATION_FWD_STREAM, +}; +typedef struct _qemuMigrationSpec qemuMigrationSpec; +typedef qemuMigrationSpec *qemuMigrationSpecPtr; +struct _qemuMigrationSpec { + enum qemuMigrationDestinationType destType; + union { + struct { + const char *name; + int port; + } host; + + struct { + const char *file; + int sock; + } unics; /* this sucks but "unix" is a macro defined to 1 */ + } dest; + + enum qemuMigrationForwardType fwdType; + union { + virStreamPtr stream; + } fwd; +}; #define TUNNEL_SEND_BUF_SIZE 65536 @@ -1483,101 +1398,33 @@ cleanup: return rv; } - -static int doTunnelMigrate(struct qemud_driver *driver, - virDomainObjPtr vm, - virStreamPtr st, - const char *cookiein, - int cookieinlen, - char **cookieout, - int *cookieoutlen, - unsigned long flags, - unsigned long resource) +static int +qemuMigrationRun(struct qemud_driver *driver, + virDomainObjPtr vm, + const char *cookiein, + int cookieinlen, + char **cookieout, + int *cookieoutlen, + unsigned int flags, + unsigned long resource, + qemuMigrationSpecPtr spec) { - qemuDomainObjPrivatePtr priv = vm->privateData; - int client_sock = -1; - int qemu_sock = -1; - struct sockaddr_un sa_qemu, sa_client; - socklen_t addrlen; - int status; - unsigned long long transferred, remaining, total; - char *unixfile = NULL; - unsigned int background_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; int ret = -1; + unsigned int migrate_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; + qemuDomainObjPrivatePtr priv = vm->privateData; qemuMigrationCookiePtr mig = NULL; qemuMigrationIOThreadPtr iothread = NULL; - VIR_DEBUG("driver=%p, vm=%p, st=%p, cookiein=%s, cookieinlen=%d, " - "cookieout=%p, cookieoutlen=%p, flags=%lx, resource=%lu", - driver, vm, st, NULLSTR(cookiein), cookieinlen, - cookieout, cookieoutlen, flags, resource); + int fd = -1; if (virLockManagerPluginUsesState(driver->lockManager) && !cookieout) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("Migration with lock driver %s requires cookie support"), + _("Migration with lock driver %s requires" + " cookie support"), virLockManagerPluginGetName(driver->lockManager)); return -1; } - if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX) && - !qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_EXEC)) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("Source qemu is too old to support tunnelled migration")); - goto cleanup; - } - - - if (virAsprintf(&unixfile, "%s/qemu.tunnelmigrate.src.%s", - driver->libDir, vm->def->name) < 0) { - virReportOOMError(); - goto cleanup; - } - - qemu_sock = socket(AF_UNIX, SOCK_STREAM, 0); - if (qemu_sock < 0) { - virReportSystemError(errno, "%s", - _("cannot open tunnelled migration socket")); - goto cleanup; - } - memset(&sa_qemu, 0, sizeof(sa_qemu)); - sa_qemu.sun_family = AF_UNIX; - if (virStrcpy(sa_qemu.sun_path, unixfile, - sizeof(sa_qemu.sun_path)) == NULL) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("Unix socket '%s' too big for destination"), - unixfile); - goto cleanup; - } - unlink(unixfile); - if (bind(qemu_sock, (struct sockaddr *)&sa_qemu, sizeof(sa_qemu)) < 0) { - virReportSystemError(errno, - _("Cannot bind to unix socket '%s' for tunnelled migration"), - unixfile); - goto cleanup; - } - if (listen(qemu_sock, 1) < 0) { - virReportSystemError(errno, - _("Cannot listen on unix socket '%s' for tunnelled migration"), - unixfile); - goto cleanup; - } - - if (chown(unixfile, driver->user, driver->group) < 0) { - virReportSystemError(errno, - _("Cannot change unix socket '%s' owner"), - unixfile); - goto cleanup; - } - - /* the domain may have shutdown or crashed while we had the locks dropped - * in qemuDomainObjEnterRemoteWithDriver, so check again - */ - if (!virDomainObjIsActive(vm)) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest unexpectedly quit")); - goto cleanup; - } - if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen, QEMU_MIGRATION_COOKIE_GRAPHICS))) goto cleanup; @@ -1585,8 +1432,7 @@ static int doTunnelMigrate(struct qemud_driver *driver, if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig) < 0) VIR_WARN("unable to provide data for graphics client relocation"); - /* 3. start migration on source */ - /* Before EnterMonitor, since qemuProcessStopCPUs already does that */ + /* Before EnterMonitor, since qemuMigrationSetOffline already does that */ if (!(flags & VIR_MIGRATE_LIVE) && virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { if (qemuMigrationSetOffline(driver, vm) < 0) @@ -1604,25 +1450,31 @@ static int doTunnelMigrate(struct qemud_driver *driver, } if (flags & VIR_MIGRATE_NON_SHARED_DISK) - background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_DISK; + migrate_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_DISK; + if (flags & VIR_MIGRATE_NON_SHARED_INC) - background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_INC; - - if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX)) { - ret = qemuMonitorMigrateToUnix(priv->mon, background_flags, - unixfile); - } else if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_EXEC)) { - const char *args[] = { "nc", "-U", unixfile, NULL }; - ret = qemuMonitorMigrateToCommand(priv->mon, QEMU_MONITOR_MIGRATE_BACKGROUND, args); - } else { - ret = -1; + migrate_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_INC; + + switch (spec->destType) { + case MIGRATION_DEST_HOST: + ret = qemuMonitorMigrateToHost(priv->mon, migrate_flags, + spec->dest.host.name, + spec->dest.host.port); + break; + + case MIGRATION_DEST_UNIX: + if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX)) { + ret = qemuMonitorMigrateToUnix(priv->mon, migrate_flags, + spec->dest.unics.file); + } else { + const char *args[] = { "nc", "-U", spec->dest.unics.file, NULL }; + ret = qemuMonitorMigrateToCommand(priv->mon, migrate_flags, args); + } + break; } qemuDomainObjExitMonitorWithDriver(driver, vm); - if (ret < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("tunnelled migration monitor command failed")); + if (ret < 0) goto cleanup; - } ret = -1; if (!virDomainObjIsActive(vm)) { @@ -1634,42 +1486,31 @@ static int doTunnelMigrate(struct qemud_driver *driver, /* From this point onwards we *must* call cancel to abort the * migration on source if anything goes wrong */ - /* it is also possible that the migrate didn't fail initially, but - * rather failed later on. Check the output of "info migrate" - */ - if (qemuDomainObjEnterMonitorAsync(driver, vm, - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) - goto cancel; - if (qemuMonitorGetMigrationStatus(priv->mon, - &status, - &transferred, - &remaining, - &total) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto cancel; - } - qemuDomainObjExitMonitorWithDriver(driver, vm); - - if (status == QEMU_MONITOR_MIGRATION_STATUS_ERROR) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s",_("migrate failed")); - goto cancel; - } + if (spec->destType == MIGRATION_DEST_UNIX) { + /* It is also possible that the migrate didn't fail initially, but + * rather failed later on. Check its status before waiting for a + * connection from qemu which may never be initiated. + */ + if (qemuMigrationUpdateJobStatus(driver, vm, _("migration job"), + QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) + goto cancel; - addrlen = sizeof(sa_client); - while ((client_sock = accept(qemu_sock, (struct sockaddr *)&sa_client, &addrlen)) < 0) { - if (errno == EAGAIN || errno == EINTR) - continue; - virReportSystemError(errno, "%s", - _("tunnelled migration failed to accept from qemu")); - goto cancel; + while ((fd = accept(spec->dest.unics.sock, NULL, NULL)) < 0) { + if (errno == EAGAIN || errno == EINTR) + continue; + virReportSystemError(errno, "%s", + _("failed to accept connection from qemu")); + goto cancel; + } } - if (!(iothread = qemuMigrationStartTunnel(st, client_sock))) + if (spec->fwdType != MIGRATION_FWD_DIRECT && + !(iothread = qemuMigrationStartTunnel(spec->fwd.stream, fd))) goto cancel; - ret = qemuMigrationWaitForCompletion(driver, vm, - QEMU_ASYNC_JOB_MIGRATION_OUT); + if (qemuMigrationWaitForCompletion(driver, vm, + QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) + goto cleanup; /* When migration completed, QEMU will have paused the * CPUs for us, but unless we're using the JSON monitor @@ -1684,30 +1525,169 @@ static int doTunnelMigrate(struct qemud_driver *driver, goto cleanup; } - /* Close now to ensure the IO thread quits & is joinable in next method */ - VIR_FORCE_CLOSE(client_sock); + ret = 0; - if (qemuMigrationStopTunnel(iothread) < 0) - ret = -1; +cleanup: + if (spec->fwdType != MIGRATION_FWD_DIRECT) { + /* Close now to ensure the IO thread quits & is joinable */ + VIR_FORCE_CLOSE(fd); + if (iothread && qemuMigrationStopTunnel(iothread) < 0) + ret = -1; + } if (ret == 0 && qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen, 0) < 0) VIR_WARN("Unable to encode migration cookie"); + qemuMigrationCookieFree(mig); + + return ret; + cancel: - if (ret != 0 && virDomainObjIsActive(vm)) { - VIR_FORCE_CLOSE(client_sock); - VIR_FORCE_CLOSE(qemu_sock); + if (virDomainObjIsActive(vm)) { if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) { qemuMonitorMigrateCancel(priv->mon); qemuDomainObjExitMonitorWithDriver(driver, vm); } } + goto cleanup; +} + +/* Perform migration using QEMU's native TCP migrate support, + * not encrypted obviously + */ +static int doNativeMigrate(struct qemud_driver *driver, + virDomainObjPtr vm, + const char *uri, + const char *cookiein, + int cookieinlen, + char **cookieout, + int *cookieoutlen, + unsigned int flags, + unsigned long resource) +{ + xmlURIPtr uribits = NULL; + int ret; + qemuMigrationSpec spec; + + VIR_DEBUG("driver=%p, vm=%p, uri=%s, cookiein=%s, cookieinlen=%d, " + "cookieout=%p, cookieoutlen=%p, flags=%x, resource=%lu", + driver, vm, uri, NULLSTR(cookiein), cookieinlen, + cookieout, cookieoutlen, flags, resource); + + if (STRPREFIX(uri, "tcp:") && !STRPREFIX(uri, "tcp://")) { + /* HACK: source host generates bogus URIs, so fix them up */ + char *tmpuri; + if (virAsprintf(&tmpuri, "tcp://%s", uri + strlen("tcp:")) < 0) { + virReportOOMError(); + return -1; + } + uribits = xmlParseURI(tmpuri); + VIR_FREE(tmpuri); + } else { + uribits = xmlParseURI(uri); + } + if (!uribits) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse URI %s"), uri); + return -1; + } + + spec.destType = MIGRATION_DEST_HOST; + spec.dest.host.name = uribits->server; + spec.dest.host.port = uribits->port; + spec.fwdType = MIGRATION_FWD_DIRECT; + + ret = qemuMigrationRun(driver, vm, cookiein, cookieinlen, cookieout, + cookieoutlen, flags, resource, &spec); + xmlFreeURI(uribits); + + return ret; +} + + +static int doTunnelMigrate(struct qemud_driver *driver, + virDomainObjPtr vm, + virStreamPtr st, + const char *cookiein, + int cookieinlen, + char **cookieout, + int *cookieoutlen, + unsigned long flags, + unsigned long resource) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int qemu_sock = -1; + struct sockaddr_un sa_qemu; + char *unixfile = NULL; + int ret = -1; + qemuMigrationSpec spec; + + VIR_DEBUG("driver=%p, vm=%p, st=%p, cookiein=%s, cookieinlen=%d, " + "cookieout=%p, cookieoutlen=%p, flags=%lx, resource=%lu", + driver, vm, st, NULLSTR(cookiein), cookieinlen, + cookieout, cookieoutlen, flags, resource); + + if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX) && + !qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_EXEC)) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("Source qemu is too old to support tunnelled migration")); + goto cleanup; + } + + if (virAsprintf(&unixfile, "%s/qemu.tunnelmigrate.src.%s", + driver->libDir, vm->def->name) < 0) { + virReportOOMError(); + goto cleanup; + } + + qemu_sock = socket(AF_UNIX, SOCK_STREAM, 0); + if (qemu_sock < 0) { + virReportSystemError(errno, "%s", + _("cannot open tunnelled migration socket")); + goto cleanup; + } + memset(&sa_qemu, 0, sizeof(sa_qemu)); + sa_qemu.sun_family = AF_UNIX; + if (virStrcpy(sa_qemu.sun_path, unixfile, + sizeof(sa_qemu.sun_path)) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Unix socket '%s' too big for destination"), + unixfile); + goto cleanup; + } + unlink(unixfile); + if (bind(qemu_sock, (struct sockaddr *)&sa_qemu, sizeof(sa_qemu)) < 0) { + virReportSystemError(errno, + _("Cannot bind to unix socket '%s' for tunnelled migration"), + unixfile); + goto cleanup; + } + if (listen(qemu_sock, 1) < 0) { + virReportSystemError(errno, + _("Cannot listen on unix socket '%s' for tunnelled migration"), + unixfile); + goto cleanup; + } + + if (chown(unixfile, driver->user, driver->group) < 0) { + virReportSystemError(errno, + _("Cannot change unix socket '%s' owner"), + unixfile); + goto cleanup; + } + + spec.destType = MIGRATION_DEST_UNIX; + spec.dest.unics.file = unixfile; + spec.dest.unics.sock = qemu_sock; + spec.fwdType = MIGRATION_FWD_STREAM; + spec.fwd.stream = st; + + ret = qemuMigrationRun(driver, vm, cookiein, cookieinlen, cookieout, + cookieoutlen, flags, resource, &spec); cleanup: - qemuMigrationCookieFree(mig); - VIR_FORCE_CLOSE(client_sock); VIR_FORCE_CLOSE(qemu_sock); if (unixfile) { unlink(unixfile); @@ -1811,7 +1791,7 @@ static int doPeer2PeerMigrate2(struct qemud_driver *driver, ret = doNativeMigrate(driver, vm, uri_out, cookie, cookielen, NULL, NULL, /* No out cookie with v2 migration */ - flags, dname, resource); + flags, resource); /* Perform failed. Make sure Finish doesn't overwrite the error */ if (ret < 0) @@ -1957,7 +1937,7 @@ static int doPeer2PeerMigrate3(struct qemud_driver *driver, ret = doNativeMigrate(driver, vm, uri_out, cookiein, cookieinlen, &cookieout, &cookieoutlen, - flags, dname, resource); + flags, resource); /* Perform failed. Make sure Finish doesn't overwrite the error */ if (ret < 0) { @@ -2185,7 +2165,7 @@ qemuMigrationPerformJob(struct qemud_driver *driver, qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM2); ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, cookieout, cookieoutlen, - flags, dname, resource); + flags, resource); } if (ret < 0) goto endjob; @@ -2254,7 +2234,6 @@ qemuMigrationPerformPhase(struct qemud_driver *driver, char **cookieout, int *cookieoutlen, unsigned long flags, - const char *dname, unsigned long resource) { virDomainEventPtr event = NULL; @@ -2275,7 +2254,7 @@ qemuMigrationPerformPhase(struct qemud_driver *driver, resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING; ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, cookieout, cookieoutlen, - flags, dname, resource); + flags, resource); if (ret < 0 && resume && virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) { @@ -2367,7 +2346,7 @@ qemuMigrationPerform(struct qemud_driver *driver, return qemuMigrationPerformPhase(driver, conn, vm, uri, cookiein, cookieinlen, cookieout, cookieoutlen, - flags, dname, resource); + flags, resource); } else { return qemuMigrationPerformJob(driver, conn, vm, xmlin, dconnuri, uri, cookiein, cookieinlen, -- 1.7.6
On Mon, Aug 15, 2011 at 09:58:14AM +0200, Jiri Denemark wrote:
The core of these two functions is very similar and most of it is even exactly the same. Factor out the core functionality into a separate function to remove code duplication and make further changes easier. --- src/qemu/qemu_migration.c | 499 ++++++++++++++++++++++----------------------- 1 files changed, 239 insertions(+), 260 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4d0e062..1cabbe0 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1265,122 +1265,37 @@ cleanup: }
-/* Perform migration using QEMU's native TCP migrate support, - * not encrypted obviously - */ -static int doNativeMigrate(struct qemud_driver *driver, - virDomainObjPtr vm, - const char *uri, - const char *cookiein, - int cookieinlen, - char **cookieout, - int *cookieoutlen, - unsigned int flags, - const char *dname ATTRIBUTE_UNUSED, - unsigned long resource) -{ - int ret = -1; - xmlURIPtr uribits = NULL; - qemuDomainObjPrivatePtr priv = vm->privateData; - unsigned int background_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; - qemuMigrationCookiePtr mig = NULL; - VIR_DEBUG("driver=%p, vm=%p, uri=%s, cookiein=%s, cookieinlen=%d, " - "cookieout=%p, cookieoutlen=%p, flags=%x, dname=%s, resource=%lu", - driver, vm, uri, NULLSTR(cookiein), cookieinlen, - cookieout, cookieoutlen, flags, NULLSTR(dname), resource); - - if (virLockManagerPluginUsesState(driver->lockManager) && - !cookieout) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("Migration with lock driver %s requires cookie support"), - virLockManagerPluginGetName(driver->lockManager)); - return -1; - } - - if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen, - QEMU_MIGRATION_COOKIE_GRAPHICS))) - goto cleanup; - - if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig) < 0) - VIR_WARN("unable to provide data for graphics client relocation"); - - /* Issue the migrate command. */ - if (STRPREFIX(uri, "tcp:") && !STRPREFIX(uri, "tcp://")) { - /* HACK: source host generates bogus URIs, so fix them up */ - char *tmpuri; - if (virAsprintf(&tmpuri, "tcp://%s", uri + strlen("tcp:")) < 0) { - virReportOOMError(); - goto cleanup; - } - uribits = xmlParseURI(tmpuri); - VIR_FREE(tmpuri); - } else { - uribits = xmlParseURI(uri); - } - if (!uribits) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse URI %s"), uri); - goto cleanup; - } - - /* Before EnterMonitor, since qemuProcessStopCPUs already does that */ - if (!(flags & VIR_MIGRATE_LIVE) && - virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - if (qemuMigrationSetOffline(driver, vm) < 0) - goto cleanup; - } - - if (qemuDomainObjEnterMonitorAsync(driver, vm, - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) - goto cleanup; - - if (resource > 0 && - qemuMonitorSetMigrationSpeed(priv->mon, resource) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto cleanup; - } - - if (flags & VIR_MIGRATE_NON_SHARED_DISK) - background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_DISK; - - if (flags & VIR_MIGRATE_NON_SHARED_INC) - background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_INC; - - if (qemuMonitorMigrateToHost(priv->mon, background_flags, uribits->server, - uribits->port) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto cleanup; - } - qemuDomainObjExitMonitorWithDriver(driver, vm); - - if (qemuMigrationWaitForCompletion(driver, vm, - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) - goto cleanup; - - /* When migration completed, QEMU will have paused the - * CPUs for us, but unless we're using the JSON monitor - * we won't have been notified of this, so might still - * think we're running. For v2 protocol this doesn't - * matter because we'll kill the VM soon, but for v3 - * this is important because we stay paused until the - * confirm3 step, but need to release the lock state - */ - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - if (qemuMigrationSetOffline(driver, vm) < 0) - goto cleanup; - } - - if (qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen, 0) < 0) - VIR_WARN("Unable to encode migration cookie"); - - ret = 0; +enum qemuMigrationDestinationType { + MIGRATION_DEST_HOST, + MIGRATION_DEST_UNIX, +};
-cleanup: - qemuMigrationCookieFree(mig); - xmlFreeURI(uribits); - return ret; -} +enum qemuMigrationForwardType { + MIGRATION_FWD_DIRECT, + MIGRATION_FWD_STREAM, +};
+typedef struct _qemuMigrationSpec qemuMigrationSpec; +typedef qemuMigrationSpec *qemuMigrationSpecPtr; +struct _qemuMigrationSpec { + enum qemuMigrationDestinationType destType; + union { + struct { + const char *name; + int port; + } host; + + struct { + const char *file; + int sock; + } unics; /* this sucks but "unix" is a macro defined to 1 */
stylistic: I would go for unx, unics is so reminiscent of Multics <sigh/> or unix_socket, that one should be clean.
+ } dest; + + enum qemuMigrationForwardType fwdType; + union { + virStreamPtr stream; + } fwd; +};
#define TUNNEL_SEND_BUF_SIZE 65536
@@ -1483,101 +1398,33 @@ cleanup: return rv; }
- -static int doTunnelMigrate(struct qemud_driver *driver, - virDomainObjPtr vm, - virStreamPtr st, - const char *cookiein, - int cookieinlen, - char **cookieout, - int *cookieoutlen, - unsigned long flags, - unsigned long resource) +static int +qemuMigrationRun(struct qemud_driver *driver, + virDomainObjPtr vm, + const char *cookiein, + int cookieinlen, + char **cookieout, + int *cookieoutlen, + unsigned int flags, + unsigned long resource, + qemuMigrationSpecPtr spec) { - qemuDomainObjPrivatePtr priv = vm->privateData; - int client_sock = -1; - int qemu_sock = -1; - struct sockaddr_un sa_qemu, sa_client; - socklen_t addrlen; - int status; - unsigned long long transferred, remaining, total; - char *unixfile = NULL; - unsigned int background_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; int ret = -1; + unsigned int migrate_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; + qemuDomainObjPrivatePtr priv = vm->privateData; qemuMigrationCookiePtr mig = NULL; qemuMigrationIOThreadPtr iothread = NULL; - VIR_DEBUG("driver=%p, vm=%p, st=%p, cookiein=%s, cookieinlen=%d, " - "cookieout=%p, cookieoutlen=%p, flags=%lx, resource=%lu", - driver, vm, st, NULLSTR(cookiein), cookieinlen, - cookieout, cookieoutlen, flags, resource);
I would still keep a VIR_DEBUG() even if there is one on the callers, because that code is new, not that simple, and I would go for increased tracability/verbosity in that case
+ int fd = -1;
if (virLockManagerPluginUsesState(driver->lockManager) && !cookieout) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("Migration with lock driver %s requires cookie support"), + _("Migration with lock driver %s requires" + " cookie support"), virLockManagerPluginGetName(driver->lockManager)); return -1; }
- if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX) && - !qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_EXEC)) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("Source qemu is too old to support tunnelled migration")); - goto cleanup; - } - - - if (virAsprintf(&unixfile, "%s/qemu.tunnelmigrate.src.%s", - driver->libDir, vm->def->name) < 0) { - virReportOOMError(); - goto cleanup; - } - - qemu_sock = socket(AF_UNIX, SOCK_STREAM, 0); - if (qemu_sock < 0) { - virReportSystemError(errno, "%s", - _("cannot open tunnelled migration socket")); - goto cleanup; - } - memset(&sa_qemu, 0, sizeof(sa_qemu)); - sa_qemu.sun_family = AF_UNIX; - if (virStrcpy(sa_qemu.sun_path, unixfile, - sizeof(sa_qemu.sun_path)) == NULL) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("Unix socket '%s' too big for destination"), - unixfile); - goto cleanup; - } - unlink(unixfile); - if (bind(qemu_sock, (struct sockaddr *)&sa_qemu, sizeof(sa_qemu)) < 0) { - virReportSystemError(errno, - _("Cannot bind to unix socket '%s' for tunnelled migration"), - unixfile); - goto cleanup; - } - if (listen(qemu_sock, 1) < 0) { - virReportSystemError(errno, - _("Cannot listen on unix socket '%s' for tunnelled migration"), - unixfile); - goto cleanup; - } - - if (chown(unixfile, driver->user, driver->group) < 0) { - virReportSystemError(errno, - _("Cannot change unix socket '%s' owner"), - unixfile); - goto cleanup; - } - - /* the domain may have shutdown or crashed while we had the locks dropped - * in qemuDomainObjEnterRemoteWithDriver, so check again - */ - if (!virDomainObjIsActive(vm)) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest unexpectedly quit")); - goto cleanup; - } - if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen, QEMU_MIGRATION_COOKIE_GRAPHICS))) goto cleanup; @@ -1585,8 +1432,7 @@ static int doTunnelMigrate(struct qemud_driver *driver, if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig) < 0) VIR_WARN("unable to provide data for graphics client relocation");
- /* 3. start migration on source */ - /* Before EnterMonitor, since qemuProcessStopCPUs already does that */ + /* Before EnterMonitor, since qemuMigrationSetOffline already does that */ if (!(flags & VIR_MIGRATE_LIVE) && virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { if (qemuMigrationSetOffline(driver, vm) < 0) @@ -1604,25 +1450,31 @@ static int doTunnelMigrate(struct qemud_driver *driver, }
if (flags & VIR_MIGRATE_NON_SHARED_DISK) - background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_DISK; + migrate_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_DISK; + if (flags & VIR_MIGRATE_NON_SHARED_INC) - background_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_INC; - - if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX)) { - ret = qemuMonitorMigrateToUnix(priv->mon, background_flags, - unixfile); - } else if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_EXEC)) { - const char *args[] = { "nc", "-U", unixfile, NULL }; - ret = qemuMonitorMigrateToCommand(priv->mon, QEMU_MONITOR_MIGRATE_BACKGROUND, args); - } else { - ret = -1; + migrate_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_INC; + + switch (spec->destType) { + case MIGRATION_DEST_HOST: + ret = qemuMonitorMigrateToHost(priv->mon, migrate_flags, + spec->dest.host.name, + spec->dest.host.port); + break; + + case MIGRATION_DEST_UNIX: + if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX)) { + ret = qemuMonitorMigrateToUnix(priv->mon, migrate_flags, + spec->dest.unics.file); + } else { + const char *args[] = { "nc", "-U", spec->dest.unics.file, NULL }; + ret = qemuMonitorMigrateToCommand(priv->mon, migrate_flags, args); + } + break; } qemuDomainObjExitMonitorWithDriver(driver, vm); - if (ret < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("tunnelled migration monitor command failed")); + if (ret < 0) goto cleanup; - } ret = -1;
if (!virDomainObjIsActive(vm)) { @@ -1634,42 +1486,31 @@ static int doTunnelMigrate(struct qemud_driver *driver, /* From this point onwards we *must* call cancel to abort the * migration on source if anything goes wrong */
- /* it is also possible that the migrate didn't fail initially, but - * rather failed later on. Check the output of "info migrate" - */ - if (qemuDomainObjEnterMonitorAsync(driver, vm, - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) - goto cancel; - if (qemuMonitorGetMigrationStatus(priv->mon, - &status, - &transferred, - &remaining, - &total) < 0) { - qemuDomainObjExitMonitorWithDriver(driver, vm); - goto cancel; - } - qemuDomainObjExitMonitorWithDriver(driver, vm); - - if (status == QEMU_MONITOR_MIGRATION_STATUS_ERROR) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s",_("migrate failed")); - goto cancel; - } + if (spec->destType == MIGRATION_DEST_UNIX) { + /* It is also possible that the migrate didn't fail initially, but + * rather failed later on. Check its status before waiting for a + * connection from qemu which may never be initiated. + */ + if (qemuMigrationUpdateJobStatus(driver, vm, _("migration job"), + QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) + goto cancel;
- addrlen = sizeof(sa_client); - while ((client_sock = accept(qemu_sock, (struct sockaddr *)&sa_client, &addrlen)) < 0) { - if (errno == EAGAIN || errno == EINTR) - continue; - virReportSystemError(errno, "%s", - _("tunnelled migration failed to accept from qemu")); - goto cancel; + while ((fd = accept(spec->dest.unics.sock, NULL, NULL)) < 0) { + if (errno == EAGAIN || errno == EINTR) + continue; + virReportSystemError(errno, "%s", + _("failed to accept connection from qemu")); + goto cancel; + } }
- if (!(iothread = qemuMigrationStartTunnel(st, client_sock))) + if (spec->fwdType != MIGRATION_FWD_DIRECT && + !(iothread = qemuMigrationStartTunnel(spec->fwd.stream, fd))) goto cancel;
- ret = qemuMigrationWaitForCompletion(driver, vm, - QEMU_ASYNC_JOB_MIGRATION_OUT); + if (qemuMigrationWaitForCompletion(driver, vm, + QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) + goto cleanup;
/* When migration completed, QEMU will have paused the * CPUs for us, but unless we're using the JSON monitor @@ -1684,30 +1525,169 @@ static int doTunnelMigrate(struct qemud_driver *driver, goto cleanup; }
- /* Close now to ensure the IO thread quits & is joinable in next method */ - VIR_FORCE_CLOSE(client_sock); + ret = 0;
- if (qemuMigrationStopTunnel(iothread) < 0) - ret = -1; +cleanup: + if (spec->fwdType != MIGRATION_FWD_DIRECT) { + /* Close now to ensure the IO thread quits & is joinable */ + VIR_FORCE_CLOSE(fd); + if (iothread && qemuMigrationStopTunnel(iothread) < 0) + ret = -1; + }
if (ret == 0 && qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen, 0) < 0) VIR_WARN("Unable to encode migration cookie");
+ qemuMigrationCookieFree(mig); + + return ret; + cancel: - if (ret != 0 && virDomainObjIsActive(vm)) { - VIR_FORCE_CLOSE(client_sock); - VIR_FORCE_CLOSE(qemu_sock); + if (virDomainObjIsActive(vm)) { if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) { qemuMonitorMigrateCancel(priv->mon); qemuDomainObjExitMonitorWithDriver(driver, vm); } } + goto cleanup; +} + +/* Perform migration using QEMU's native TCP migrate support, + * not encrypted obviously + */ +static int doNativeMigrate(struct qemud_driver *driver, + virDomainObjPtr vm, + const char *uri, + const char *cookiein, + int cookieinlen, + char **cookieout, + int *cookieoutlen, + unsigned int flags, + unsigned long resource) +{ + xmlURIPtr uribits = NULL; + int ret; + qemuMigrationSpec spec; + + VIR_DEBUG("driver=%p, vm=%p, uri=%s, cookiein=%s, cookieinlen=%d, " + "cookieout=%p, cookieoutlen=%p, flags=%x, resource=%lu", + driver, vm, uri, NULLSTR(cookiein), cookieinlen, + cookieout, cookieoutlen, flags, resource); + + if (STRPREFIX(uri, "tcp:") && !STRPREFIX(uri, "tcp://")) { + /* HACK: source host generates bogus URIs, so fix them up */ + char *tmpuri; + if (virAsprintf(&tmpuri, "tcp://%s", uri + strlen("tcp:")) < 0) { + virReportOOMError(); + return -1; + } + uribits = xmlParseURI(tmpuri); + VIR_FREE(tmpuri); + } else { + uribits = xmlParseURI(uri); + } + if (!uribits) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse URI %s"), uri); + return -1; + } + + spec.destType = MIGRATION_DEST_HOST; + spec.dest.host.name = uribits->server; + spec.dest.host.port = uribits->port; + spec.fwdType = MIGRATION_FWD_DIRECT; + + ret = qemuMigrationRun(driver, vm, cookiein, cookieinlen, cookieout, + cookieoutlen, flags, resource, &spec); + xmlFreeURI(uribits); + + return ret; +} + + +static int doTunnelMigrate(struct qemud_driver *driver, + virDomainObjPtr vm, + virStreamPtr st, + const char *cookiein, + int cookieinlen, + char **cookieout, + int *cookieoutlen, + unsigned long flags, + unsigned long resource) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int qemu_sock = -1; + struct sockaddr_un sa_qemu; + char *unixfile = NULL; + int ret = -1; + qemuMigrationSpec spec; + + VIR_DEBUG("driver=%p, vm=%p, st=%p, cookiein=%s, cookieinlen=%d, " + "cookieout=%p, cookieoutlen=%p, flags=%lx, resource=%lu", + driver, vm, st, NULLSTR(cookiein), cookieinlen, + cookieout, cookieoutlen, flags, resource); + + if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX) && + !qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_EXEC)) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + "%s", _("Source qemu is too old to support tunnelled migration")); + goto cleanup; + } + + if (virAsprintf(&unixfile, "%s/qemu.tunnelmigrate.src.%s", + driver->libDir, vm->def->name) < 0) { + virReportOOMError(); + goto cleanup; + } + + qemu_sock = socket(AF_UNIX, SOCK_STREAM, 0); + if (qemu_sock < 0) { + virReportSystemError(errno, "%s", + _("cannot open tunnelled migration socket")); + goto cleanup; + } + memset(&sa_qemu, 0, sizeof(sa_qemu)); + sa_qemu.sun_family = AF_UNIX; + if (virStrcpy(sa_qemu.sun_path, unixfile, + sizeof(sa_qemu.sun_path)) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Unix socket '%s' too big for destination"), + unixfile); + goto cleanup; + } + unlink(unixfile); + if (bind(qemu_sock, (struct sockaddr *)&sa_qemu, sizeof(sa_qemu)) < 0) { + virReportSystemError(errno, + _("Cannot bind to unix socket '%s' for tunnelled migration"), + unixfile); + goto cleanup; + } + if (listen(qemu_sock, 1) < 0) { + virReportSystemError(errno, + _("Cannot listen on unix socket '%s' for tunnelled migration"), + unixfile); + goto cleanup; + } + + if (chown(unixfile, driver->user, driver->group) < 0) { + virReportSystemError(errno, + _("Cannot change unix socket '%s' owner"), + unixfile); + goto cleanup; + } + + spec.destType = MIGRATION_DEST_UNIX; + spec.dest.unics.file = unixfile; + spec.dest.unics.sock = qemu_sock; + spec.fwdType = MIGRATION_FWD_STREAM; + spec.fwd.stream = st; + + ret = qemuMigrationRun(driver, vm, cookiein, cookieinlen, cookieout, + cookieoutlen, flags, resource, &spec);
cleanup: - qemuMigrationCookieFree(mig); - VIR_FORCE_CLOSE(client_sock); VIR_FORCE_CLOSE(qemu_sock); if (unixfile) { unlink(unixfile); @@ -1811,7 +1791,7 @@ static int doPeer2PeerMigrate2(struct qemud_driver *driver, ret = doNativeMigrate(driver, vm, uri_out, cookie, cookielen, NULL, NULL, /* No out cookie with v2 migration */ - flags, dname, resource); + flags, resource);
/* Perform failed. Make sure Finish doesn't overwrite the error */ if (ret < 0) @@ -1957,7 +1937,7 @@ static int doPeer2PeerMigrate3(struct qemud_driver *driver, ret = doNativeMigrate(driver, vm, uri_out, cookiein, cookieinlen, &cookieout, &cookieoutlen, - flags, dname, resource); + flags, resource);
/* Perform failed. Make sure Finish doesn't overwrite the error */ if (ret < 0) { @@ -2185,7 +2165,7 @@ qemuMigrationPerformJob(struct qemud_driver *driver, qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM2); ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, cookieout, cookieoutlen, - flags, dname, resource); + flags, resource); } if (ret < 0) goto endjob; @@ -2254,7 +2234,6 @@ qemuMigrationPerformPhase(struct qemud_driver *driver, char **cookieout, int *cookieoutlen, unsigned long flags, - const char *dname, unsigned long resource) { virDomainEventPtr event = NULL; @@ -2275,7 +2254,7 @@ qemuMigrationPerformPhase(struct qemud_driver *driver, resume = virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING; ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, cookieout, cookieoutlen, - flags, dname, resource); + flags, resource);
if (ret < 0 && resume && virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) { @@ -2367,7 +2346,7 @@ qemuMigrationPerform(struct qemud_driver *driver, return qemuMigrationPerformPhase(driver, conn, vm, uri, cookiein, cookieinlen, cookieout, cookieoutlen, - flags, dname, resource); + flags, resource); } else { return qemuMigrationPerformJob(driver, conn, vm, xmlin, dconnuri, uri, cookiein, cookieinlen,
It's a bit hard to really prove that there is no semantic change in the refactoring, but the principle sounds good, and it looks okay. ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, Aug 15, 2011 at 17:48:03 +0800, Daniel Veillard wrote:
On Mon, Aug 15, 2011 at 09:58:14AM +0200, Jiri Denemark wrote:
The core of these two functions is very similar and most of it is even exactly the same. Factor out the core functionality into a separate function to remove code duplication and make further changes easier. --- src/qemu/qemu_migration.c | 499 ++++++++++++++++++++++----------------------- 1 files changed, 239 insertions(+), 260 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4d0e062..1cabbe0 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1265,122 +1265,37 @@ cleanup: ... +struct _qemuMigrationSpec { + enum qemuMigrationDestinationType destType; + union { + struct { + const char *name; + int port; + } host; + + struct { + const char *file; + int sock; + } unics; /* this sucks but "unix" is a macro defined to 1 */
stylistic: I would go for unx, unics is so reminiscent of Multics <sigh/> or unix_socket, that one should be clean.
OK, I kind of like this link to Multics :-) but I agree that unix_socket is cleaner (though unix_socket.sock is not so nice as unix.sock would have been) and I also removed the comment. ...
+qemuMigrationRun(struct qemud_driver *driver, + virDomainObjPtr vm, + const char *cookiein, + int cookieinlen, + char **cookieout, + int *cookieoutlen, + unsigned int flags, + unsigned long resource, + qemuMigrationSpecPtr spec) { - qemuDomainObjPrivatePtr priv = vm->privateData; - int client_sock = -1; - int qemu_sock = -1; - struct sockaddr_un sa_qemu, sa_client; - socklen_t addrlen; - int status; - unsigned long long transferred, remaining, total; - char *unixfile = NULL; - unsigned int background_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; int ret = -1; + unsigned int migrate_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; + qemuDomainObjPrivatePtr priv = vm->privateData; qemuMigrationCookiePtr mig = NULL; qemuMigrationIOThreadPtr iothread = NULL; - VIR_DEBUG("driver=%p, vm=%p, st=%p, cookiein=%s, cookieinlen=%d, " - "cookieout=%p, cookieoutlen=%p, flags=%lx, resource=%lu", - driver, vm, st, NULLSTR(cookiein), cookieinlen, - cookieout, cookieoutlen, flags, resource);
I would still keep a VIR_DEBUG() even if there is one on the callers, because that code is new, not that simple, and I would go for increased tracability/verbosity in that case
OK, I added this additional VIR_DEBUG to qemuMigrationRun too. Note that the existing do{Native,Tunnel}Migrate didn't lose their VIR_DEBUG calls. Jirka
--- src/qemu/qemu_migration.c | 50 +++++++------------------------------------- 1 files changed, 8 insertions(+), 42 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 1cabbe0..c29ea9e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -42,6 +42,7 @@ #include "fdstream.h" #include "uuid.h" #include "locking/domain_lock.h" +#include "rpc/virnetsocket.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -1618,9 +1619,8 @@ static int doTunnelMigrate(struct qemud_driver *driver, unsigned long resource) { qemuDomainObjPrivatePtr priv = vm->privateData; - int qemu_sock = -1; - struct sockaddr_un sa_qemu; char *unixfile = NULL; + virNetSocketPtr sock = NULL; int ret = -1; qemuMigrationSpec spec; @@ -1642,45 +1642,14 @@ static int doTunnelMigrate(struct qemud_driver *driver, goto cleanup; } - qemu_sock = socket(AF_UNIX, SOCK_STREAM, 0); - if (qemu_sock < 0) { - virReportSystemError(errno, "%s", - _("cannot open tunnelled migration socket")); - goto cleanup; - } - memset(&sa_qemu, 0, sizeof(sa_qemu)); - sa_qemu.sun_family = AF_UNIX; - if (virStrcpy(sa_qemu.sun_path, unixfile, - sizeof(sa_qemu.sun_path)) == NULL) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("Unix socket '%s' too big for destination"), - unixfile); + if (virNetSocketNewListenUNIX(unixfile, 0700, + driver->user, driver->group, &sock) < 0 || + virNetSocketListen(sock, 1) < 0) goto cleanup; - } - unlink(unixfile); - if (bind(qemu_sock, (struct sockaddr *)&sa_qemu, sizeof(sa_qemu)) < 0) { - virReportSystemError(errno, - _("Cannot bind to unix socket '%s' for tunnelled migration"), - unixfile); - goto cleanup; - } - if (listen(qemu_sock, 1) < 0) { - virReportSystemError(errno, - _("Cannot listen on unix socket '%s' for tunnelled migration"), - unixfile); - goto cleanup; - } - - if (chown(unixfile, driver->user, driver->group) < 0) { - virReportSystemError(errno, - _("Cannot change unix socket '%s' owner"), - unixfile); - goto cleanup; - } spec.destType = MIGRATION_DEST_UNIX; spec.dest.unics.file = unixfile; - spec.dest.unics.sock = qemu_sock; + spec.dest.unics.sock = virNetSocketGetFD(sock); spec.fwdType = MIGRATION_FWD_STREAM; spec.fwd.stream = st; @@ -1688,11 +1657,8 @@ static int doTunnelMigrate(struct qemud_driver *driver, cookieoutlen, flags, resource, &spec); cleanup: - VIR_FORCE_CLOSE(qemu_sock); - if (unixfile) { - unlink(unixfile); - VIR_FREE(unixfile); - } + virNetSocketFree(sock); + VIR_FREE(unixfile); return ret; } -- 1.7.6
On Mon, Aug 15, 2011 at 09:58:15AM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_migration.c | 50 +++++++------------------------------------- 1 files changed, 8 insertions(+), 42 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 1cabbe0..c29ea9e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -42,6 +42,7 @@ #include "fdstream.h" #include "uuid.h" #include "locking/domain_lock.h" +#include "rpc/virnetsocket.h"
#define VIR_FROM_THIS VIR_FROM_QEMU @@ -1618,9 +1619,8 @@ static int doTunnelMigrate(struct qemud_driver *driver, unsigned long resource) { qemuDomainObjPrivatePtr priv = vm->privateData; - int qemu_sock = -1; - struct sockaddr_un sa_qemu; char *unixfile = NULL; + virNetSocketPtr sock = NULL; int ret = -1; qemuMigrationSpec spec;
@@ -1642,45 +1642,14 @@ static int doTunnelMigrate(struct qemud_driver *driver, goto cleanup; }
- qemu_sock = socket(AF_UNIX, SOCK_STREAM, 0); - if (qemu_sock < 0) { - virReportSystemError(errno, "%s", - _("cannot open tunnelled migration socket")); - goto cleanup; - } - memset(&sa_qemu, 0, sizeof(sa_qemu)); - sa_qemu.sun_family = AF_UNIX; - if (virStrcpy(sa_qemu.sun_path, unixfile, - sizeof(sa_qemu.sun_path)) == NULL) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("Unix socket '%s' too big for destination"), - unixfile); + if (virNetSocketNewListenUNIX(unixfile, 0700, + driver->user, driver->group, &sock) < 0 || + virNetSocketListen(sock, 1) < 0)
If we are sure the errors will be properly reported back in called functions, okay
goto cleanup; - } - unlink(unixfile); - if (bind(qemu_sock, (struct sockaddr *)&sa_qemu, sizeof(sa_qemu)) < 0) { - virReportSystemError(errno, - _("Cannot bind to unix socket '%s' for tunnelled migration"), - unixfile); - goto cleanup; - } - if (listen(qemu_sock, 1) < 0) { - virReportSystemError(errno, - _("Cannot listen on unix socket '%s' for tunnelled migration"), - unixfile); - goto cleanup; - } - - if (chown(unixfile, driver->user, driver->group) < 0) { - virReportSystemError(errno, - _("Cannot change unix socket '%s' owner"), - unixfile); - goto cleanup; - }
spec.destType = MIGRATION_DEST_UNIX; spec.dest.unics.file = unixfile; - spec.dest.unics.sock = qemu_sock; + spec.dest.unics.sock = virNetSocketGetFD(sock); spec.fwdType = MIGRATION_FWD_STREAM; spec.fwd.stream = st;
@@ -1688,11 +1657,8 @@ static int doTunnelMigrate(struct qemud_driver *driver, cookieoutlen, flags, resource, &spec);
cleanup: - VIR_FORCE_CLOSE(qemu_sock); - if (unixfile) { - unlink(unixfile); - VIR_FREE(unixfile); - } + virNetSocketFree(sock); + VIR_FREE(unixfile);
return ret; }
Looks lovely except that a code removing so many lines on migration code souds too good to be true <grin/> ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, Aug 15, 2011 at 17:55:33 +0800, Daniel Veillard wrote:
On Mon, Aug 15, 2011 at 09:58:15AM +0200, Jiri Denemark wrote:
--- src/qemu/qemu_migration.c | 50 +++++++------------------------------------- 1 files changed, 8 insertions(+), 42 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 1cabbe0..c29ea9e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -42,6 +42,7 @@ ... - qemu_sock = socket(AF_UNIX, SOCK_STREAM, 0); - if (qemu_sock < 0) { - virReportSystemError(errno, "%s", - _("cannot open tunnelled migration socket")); - goto cleanup; - } - memset(&sa_qemu, 0, sizeof(sa_qemu)); - sa_qemu.sun_family = AF_UNIX; - if (virStrcpy(sa_qemu.sun_path, unixfile, - sizeof(sa_qemu.sun_path)) == NULL) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("Unix socket '%s' too big for destination"), - unixfile); + if (virNetSocketNewListenUNIX(unixfile, 0700, + driver->user, driver->group, &sock) < 0 || + virNetSocketListen(sock, 1) < 0)
If we are sure the errors will be properly reported back in called functions, okay
Yep, both virNetSocketNewListenUNIX and virNetSocketListen report the errors themselves.
Looks lovely except that a code removing so many lines on migration code souds too good to be true <grin/>
:-P Jirka
By opening a connection to remote qemu process ourselves and passing the socket to qemu we get much better errors than just "migration failed" when the connection is opened by qemu. --- src/qemu/qemu_migration.c | 128 ++++++++++++++++++++++++++++++++++----------- 1 files changed, 98 insertions(+), 30 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index c29ea9e..537e57e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1269,6 +1269,7 @@ cleanup: enum qemuMigrationDestinationType { MIGRATION_DEST_HOST, MIGRATION_DEST_UNIX, + MIGRATION_DEST_FD, }; enum qemuMigrationForwardType { @@ -1287,9 +1288,14 @@ struct _qemuMigrationSpec { } host; struct { - const char *file; + char *file; int sock; } unics; /* this sucks but "unix" is a macro defined to 1 */ + + struct { + int qemu; + int local; + } fd; } dest; enum qemuMigrationForwardType fwdType; @@ -1472,6 +1478,14 @@ qemuMigrationRun(struct qemud_driver *driver, ret = qemuMonitorMigrateToCommand(priv->mon, migrate_flags, args); } break; + + case MIGRATION_DEST_FD: + if (spec->fwdType != MIGRATION_FWD_DIRECT) + fd = spec->dest.fd.local; + ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, + spec->dest.fd.qemu); + VIR_FORCE_CLOSE(spec->dest.fd.qemu); + break; } qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret < 0) @@ -1568,9 +1582,11 @@ static int doNativeMigrate(struct qemud_driver *driver, unsigned int flags, unsigned long resource) { + qemuDomainObjPrivatePtr priv = vm->privateData; xmlURIPtr uribits = NULL; - int ret; + int ret = -1; qemuMigrationSpec spec; + char *tmp = NULL; VIR_DEBUG("driver=%p, vm=%p, uri=%s, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, flags=%x, resource=%lu", @@ -1579,13 +1595,12 @@ static int doNativeMigrate(struct qemud_driver *driver, if (STRPREFIX(uri, "tcp:") && !STRPREFIX(uri, "tcp://")) { /* HACK: source host generates bogus URIs, so fix them up */ - char *tmpuri; - if (virAsprintf(&tmpuri, "tcp://%s", uri + strlen("tcp:")) < 0) { + if (virAsprintf(&tmp, "tcp://%s", uri + strlen("tcp:")) < 0) { virReportOOMError(); return -1; } - uribits = xmlParseURI(tmpuri); - VIR_FREE(tmpuri); + uribits = xmlParseURI(tmp); + VIR_FREE(tmp); } else { uribits = xmlParseURI(uri); } @@ -1595,13 +1610,38 @@ static int doNativeMigrate(struct qemud_driver *driver, return -1; } - spec.destType = MIGRATION_DEST_HOST; - spec.dest.host.name = uribits->server; - spec.dest.host.port = uribits->port; spec.fwdType = MIGRATION_FWD_DIRECT; + if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)) { + virNetSocketPtr sock; + + spec.destType = MIGRATION_DEST_FD; + spec.dest.fd.qemu = -1; + + if (virAsprintf(&tmp, "%d", uribits->port) < 0) { + virReportOOMError(); + goto cleanup; + } + if (virNetSocketNewConnectTCP(uribits->server, tmp, &sock) == 0) { + spec.dest.fd.qemu = virNetSocketDupFD(sock, true); + virNetSocketFree(sock); + } + if (spec.dest.fd.qemu == -1) + goto cleanup; + } else { + spec.destType = MIGRATION_DEST_HOST; + spec.dest.host.name = uribits->server; + spec.dest.host.port = uribits->port; + } + ret = qemuMigrationRun(driver, vm, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, &spec); + +cleanup: + if (spec.destType == MIGRATION_DEST_FD) + VIR_FORCE_CLOSE(spec.dest.fd.qemu); + + VIR_FREE(tmp); xmlFreeURI(uribits); return ret; @@ -1619,7 +1659,6 @@ static int doTunnelMigrate(struct qemud_driver *driver, unsigned long resource) { qemuDomainObjPrivatePtr priv = vm->privateData; - char *unixfile = NULL; virNetSocketPtr sock = NULL; int ret = -1; qemuMigrationSpec spec; @@ -1629,36 +1668,65 @@ static int doTunnelMigrate(struct qemud_driver *driver, driver, vm, st, NULLSTR(cookiein), cookieinlen, cookieout, cookieoutlen, flags, resource); - if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX) && + if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) && + !qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX) && !qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_EXEC)) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("Source qemu is too old to support tunnelled migration")); - goto cleanup; - } - - if (virAsprintf(&unixfile, "%s/qemu.tunnelmigrate.src.%s", - driver->libDir, vm->def->name) < 0) { - virReportOOMError(); - goto cleanup; + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Source qemu is too old to support tunnelled migration")); + return -1; } - if (virNetSocketNewListenUNIX(unixfile, 0700, - driver->user, driver->group, &sock) < 0 || - virNetSocketListen(sock, 1) < 0) - goto cleanup; - - spec.destType = MIGRATION_DEST_UNIX; - spec.dest.unics.file = unixfile; - spec.dest.unics.sock = virNetSocketGetFD(sock); spec.fwdType = MIGRATION_FWD_STREAM; spec.fwd.stream = st; + if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)) { + int fds[2]; + + spec.destType = MIGRATION_DEST_FD; + spec.dest.fd.qemu = -1; + spec.dest.fd.local = -1; + + if (pipe(fds) == 0) { + spec.dest.fd.qemu = fds[1]; + spec.dest.fd.local = fds[0]; + } + if (spec.dest.fd.qemu == -1 || + virSetCloseExec(spec.dest.fd.qemu) < 0 || + virSetCloseExec(spec.dest.fd.local) < 0) { + virReportSystemError(errno, "%s", + _("cannot create pipe for tunnelled migration")); + goto cleanup; + } + } else { + spec.destType = MIGRATION_DEST_UNIX; + spec.dest.unics.sock = -1; + spec.dest.unics.file = NULL; + + if (virAsprintf(&spec.dest.unics.file, "%s/qemu.tunnelmigrate.src.%s", + driver->libDir, vm->def->name) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virNetSocketNewListenUNIX(spec.dest.unics.file, 0700, driver->user, + driver->group, &sock) < 0 || + virNetSocketListen(sock, 1) < 0) + goto cleanup; + + spec.dest.unics.sock = virNetSocketGetFD(sock); + } + ret = qemuMigrationRun(driver, vm, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, &spec); cleanup: - virNetSocketFree(sock); - VIR_FREE(unixfile); + if (spec.destType == MIGRATION_DEST_FD) { + VIR_FORCE_CLOSE(spec.dest.fd.qemu); + VIR_FORCE_CLOSE(spec.dest.fd.local); + } else { + virNetSocketFree(sock); + VIR_FREE(spec.dest.unics.file); + } return ret; } -- 1.7.6
On Mon, Aug 15, 2011 at 09:58:16AM +0200, Jiri Denemark wrote:
By opening a connection to remote qemu process ourselves and passing the socket to qemu we get much better errors than just "migration failed" when the connection is opened by qemu. --- src/qemu/qemu_migration.c | 128 ++++++++++++++++++++++++++++++++++----------- 1 files changed, 98 insertions(+), 30 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index c29ea9e..537e57e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1269,6 +1269,7 @@ cleanup: enum qemuMigrationDestinationType { MIGRATION_DEST_HOST, MIGRATION_DEST_UNIX, + MIGRATION_DEST_FD, };
enum qemuMigrationForwardType { @@ -1287,9 +1288,14 @@ struct _qemuMigrationSpec { } host;
struct { - const char *file; + char *file; int sock; } unics; /* this sucks but "unix" is a macro defined to 1 */ + + struct { + int qemu; + int local; + } fd; } dest;
enum qemuMigrationForwardType fwdType; @@ -1472,6 +1478,14 @@ qemuMigrationRun(struct qemud_driver *driver, ret = qemuMonitorMigrateToCommand(priv->mon, migrate_flags, args); } break; + + case MIGRATION_DEST_FD: + if (spec->fwdType != MIGRATION_FWD_DIRECT) + fd = spec->dest.fd.local; + ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, + spec->dest.fd.qemu); + VIR_FORCE_CLOSE(spec->dest.fd.qemu);
Hum, I find dubious that we set up fd variable before calling qemuMonitorMigrateToFd but don't use that variable, smells fishy but I could be wrong since I don't get the full function context, so to double check.
+ break; } qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret < 0) @@ -1568,9 +1582,11 @@ static int doNativeMigrate(struct qemud_driver *driver, unsigned int flags, unsigned long resource) { + qemuDomainObjPrivatePtr priv = vm->privateData; xmlURIPtr uribits = NULL; - int ret; + int ret = -1; qemuMigrationSpec spec; + char *tmp = NULL;
VIR_DEBUG("driver=%p, vm=%p, uri=%s, cookiein=%s, cookieinlen=%d, " "cookieout=%p, cookieoutlen=%p, flags=%x, resource=%lu", @@ -1579,13 +1595,12 @@ static int doNativeMigrate(struct qemud_driver *driver,
if (STRPREFIX(uri, "tcp:") && !STRPREFIX(uri, "tcp://")) { /* HACK: source host generates bogus URIs, so fix them up */ - char *tmpuri; - if (virAsprintf(&tmpuri, "tcp://%s", uri + strlen("tcp:")) < 0) { + if (virAsprintf(&tmp, "tcp://%s", uri + strlen("tcp:")) < 0) { virReportOOMError(); return -1; } - uribits = xmlParseURI(tmpuri); - VIR_FREE(tmpuri); + uribits = xmlParseURI(tmp); + VIR_FREE(tmp); } else { uribits = xmlParseURI(uri); } @@ -1595,13 +1610,38 @@ static int doNativeMigrate(struct qemud_driver *driver, return -1; }
- spec.destType = MIGRATION_DEST_HOST; - spec.dest.host.name = uribits->server; - spec.dest.host.port = uribits->port; spec.fwdType = MIGRATION_FWD_DIRECT;
+ if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)) { + virNetSocketPtr sock; + + spec.destType = MIGRATION_DEST_FD; + spec.dest.fd.qemu = -1; + + if (virAsprintf(&tmp, "%d", uribits->port) < 0) { + virReportOOMError(); + goto cleanup; + } + if (virNetSocketNewConnectTCP(uribits->server, tmp, &sock) == 0) { + spec.dest.fd.qemu = virNetSocketDupFD(sock, true); + virNetSocketFree(sock); + } + if (spec.dest.fd.qemu == -1) + goto cleanup; + } else { + spec.destType = MIGRATION_DEST_HOST; + spec.dest.host.name = uribits->server; + spec.dest.host.port = uribits->port; + } + ret = qemuMigrationRun(driver, vm, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, &spec); + +cleanup: + if (spec.destType == MIGRATION_DEST_FD) + VIR_FORCE_CLOSE(spec.dest.fd.qemu); + + VIR_FREE(tmp); xmlFreeURI(uribits);
return ret; @@ -1619,7 +1659,6 @@ static int doTunnelMigrate(struct qemud_driver *driver, unsigned long resource) { qemuDomainObjPrivatePtr priv = vm->privateData; - char *unixfile = NULL; virNetSocketPtr sock = NULL; int ret = -1; qemuMigrationSpec spec; @@ -1629,36 +1668,65 @@ static int doTunnelMigrate(struct qemud_driver *driver, driver, vm, st, NULLSTR(cookiein), cookieinlen, cookieout, cookieoutlen, flags, resource);
- if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX) && + if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) && + !qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX) && !qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_EXEC)) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - "%s", _("Source qemu is too old to support tunnelled migration")); - goto cleanup; - } - - if (virAsprintf(&unixfile, "%s/qemu.tunnelmigrate.src.%s", - driver->libDir, vm->def->name) < 0) { - virReportOOMError(); - goto cleanup; + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Source qemu is too old to support tunnelled migration")); + return -1; }
- if (virNetSocketNewListenUNIX(unixfile, 0700, - driver->user, driver->group, &sock) < 0 || - virNetSocketListen(sock, 1) < 0) - goto cleanup; - - spec.destType = MIGRATION_DEST_UNIX; - spec.dest.unics.file = unixfile; - spec.dest.unics.sock = virNetSocketGetFD(sock); spec.fwdType = MIGRATION_FWD_STREAM; spec.fwd.stream = st;
+ if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)) { + int fds[2]; + + spec.destType = MIGRATION_DEST_FD; + spec.dest.fd.qemu = -1; + spec.dest.fd.local = -1; + + if (pipe(fds) == 0) { + spec.dest.fd.qemu = fds[1]; + spec.dest.fd.local = fds[0]; + } + if (spec.dest.fd.qemu == -1 || + virSetCloseExec(spec.dest.fd.qemu) < 0 || + virSetCloseExec(spec.dest.fd.local) < 0) { + virReportSystemError(errno, "%s", + _("cannot create pipe for tunnelled migration")); + goto cleanup; + } + } else { + spec.destType = MIGRATION_DEST_UNIX; + spec.dest.unics.sock = -1; + spec.dest.unics.file = NULL; + + if (virAsprintf(&spec.dest.unics.file, "%s/qemu.tunnelmigrate.src.%s", + driver->libDir, vm->def->name) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (virNetSocketNewListenUNIX(spec.dest.unics.file, 0700, driver->user, + driver->group, &sock) < 0 || + virNetSocketListen(sock, 1) < 0) + goto cleanup; + + spec.dest.unics.sock = virNetSocketGetFD(sock); + } + ret = qemuMigrationRun(driver, vm, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, &spec);
cleanup: - virNetSocketFree(sock); - VIR_FREE(unixfile); + if (spec.destType == MIGRATION_DEST_FD) { + VIR_FORCE_CLOSE(spec.dest.fd.qemu); + VIR_FORCE_CLOSE(spec.dest.fd.local); + } else { + virNetSocketFree(sock); + VIR_FREE(spec.dest.unics.file); + }
return ret; }
Okay, I guess the best is to apply and run the verious test suites on it ! Out of curiosity did you ran dan's migration suite with those patches ? ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Mon, Aug 15, 2011 at 18:04:07 +0800, Daniel Veillard wrote:
On Mon, Aug 15, 2011 at 09:58:16AM +0200, Jiri Denemark wrote:
By opening a connection to remote qemu process ourselves and passing the socket to qemu we get much better errors than just "migration failed" when the connection is opened by qemu. --- src/qemu/qemu_migration.c | 128 ++++++++++++++++++++++++++++++++++----------- 1 files changed, 98 insertions(+), 30 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index c29ea9e..537e57e 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1269,6 +1269,7 @@ cleanup: ... enum qemuMigrationDestinationType { MIGRATION_DEST_HOST, MIGRATION_DEST_UNIX, + MIGRATION_DEST_FD, };
enum qemuMigrationForwardType { @@ -1287,9 +1288,14 @@ struct _qemuMigrationSpec { } host;
struct { - const char *file; + char *file; int sock; } unics; /* this sucks but "unix" is a macro defined to 1 */ + + struct { + int qemu; + int local; + } fd; } dest;
enum qemuMigrationForwardType fwdType; @@ -1472,6 +1478,14 @@ qemuMigrationRun(struct qemud_driver *driver, ret = qemuMonitorMigrateToCommand(priv->mon, migrate_flags, args); } break; + + case MIGRATION_DEST_FD: + if (spec->fwdType != MIGRATION_FWD_DIRECT) + fd = spec->dest.fd.local; + ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags, + spec->dest.fd.qemu); + VIR_FORCE_CLOSE(spec->dest.fd.qemu);
Hum, I find dubious that we set up fd variable before calling qemuMonitorMigrateToFd but don't use that variable, smells fishy but I could be wrong since I don't get the full function context, so to double check.
The fd variable contains local end of a pipe or socket, that is it's the file descriptor we will later read migration data from and forward through virStream.
Okay, I guess the best is to apply and run the verious test suites on it ! Out of curiosity did you ran dan's migration suite with those patches ?
Unfortunately, I still don't have the setup for doing that. However, I did test all kinds (normal, p2p, tunneled) of migration after the refactoring patch, after switching unix socket creation to virNetSocket, and after this patch and all tests (after fixing some bugs and hitting some other unrelated bugs that will be fixed later) went fine. Jirka
On 08/15/2011 01:58 AM, Jiri Denemark wrote:
By opening a connection to remote qemu process ourselves and passing the socket to qemu we get much better errors than just "migration failed" when the connection is opened by qemu. --- src/qemu/qemu_migration.c | 128 ++++++++++++++++++++++++++++++++++----------- 1 files changed, 98 insertions(+), 30 deletions(-)
+ if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)) { + int fds[2]; + + spec.destType = MIGRATION_DEST_FD; + spec.dest.fd.qemu = -1; + spec.dest.fd.local = -1; + + if (pipe(fds) == 0) { + spec.dest.fd.qemu = fds[1]; + spec.dest.fd.local = fds[0]; + } + if (spec.dest.fd.qemu == -1 || + virSetCloseExec(spec.dest.fd.qemu) < 0 || + virSetCloseExec(spec.dest.fd.local) < 0) { + virReportSystemError(errno, "%s", + _("cannot create pipe for tunnelled migration"));
SELinux doesn't like this. We never label the pipe here, and qemuMonitorMigrateToFd doesn't label the outgoing pipe either. Thus, when we hand the fd to qemu for tunneled migration, SELinux rejects the first write() attempt, and qemu fails with: internal error unable to execute QEMU command 'getfd': No file descriptor supplied via SCM_RIGHTS I'm still testing this, but based on how we label the incoming pipe in qemuProcessStart, I think this will solve the problem. diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c index a2dc97c..38b05a9 100644 --- i/src/qemu/qemu_migration.c +++ w/src/qemu/qemu_migration.c @@ -24,6 +24,7 @@ #include <sys/time.h> #include <gnutls/gnutls.h> #include <gnutls/x509.h> +#include <fcntl.h> #include "qemu_migration.h" #include "qemu_monitor.h" @@ -1691,13 +1692,13 @@ static int doTunnelMigrate(struct qemud_driver *driver, spec.dest.fd.qemu = -1; spec.dest.fd.local = -1; - if (pipe(fds) == 0) { + if (pipe2(fds, O_CLOEXEC) == 0) { spec.dest.fd.qemu = fds[1]; spec.dest.fd.local = fds[0]; } if (spec.dest.fd.qemu == -1 || - virSetCloseExec(spec.dest.fd.qemu) < 0 || - virSetCloseExec(spec.dest.fd.local) < 0) { + virSecurityManagerSetImageFDLabel(driver->securityManager, vm, + spec.dest.fd.qemu) < 0) { virReportSystemError(errno, "%s", _("cannot create pipe for tunnelled migration")); goto cleanup; -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/29/2011 11:40 AM, Eric Blake wrote:
SELinux doesn't like this. We never label the pipe here, and qemuMonitorMigrateToFd doesn't label the outgoing pipe either. Thus, when we hand the fd to qemu for tunneled migration, SELinux rejects the first write() attempt, and qemu fails with:
internal error unable to execute QEMU command 'getfd': No file descriptor supplied via SCM_RIGHTS
I'm still testing this, but based on how we label the incoming pipe in qemuProcessStart, I think this will solve the problem.
My testing is complete, this did indeed fix the problem I was seeing, and with this patch installed, I was able to do a tunneled migration with SELinux enforcing.
diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c index a2dc97c..38b05a9 100644 --- i/src/qemu/qemu_migration.c +++ w/src/qemu/qemu_migration.c @@ -24,6 +24,7 @@ #include <sys/time.h> #include <gnutls/gnutls.h> #include <gnutls/x509.h> +#include <fcntl.h>
#include "qemu_migration.h" #include "qemu_monitor.h" @@ -1691,13 +1692,13 @@ static int doTunnelMigrate(struct qemud_driver *driver, spec.dest.fd.qemu = -1; spec.dest.fd.local = -1;
- if (pipe(fds) == 0) { + if (pipe2(fds, O_CLOEXEC) == 0) { spec.dest.fd.qemu = fds[1]; spec.dest.fd.local = fds[0]; } if (spec.dest.fd.qemu == -1 || - virSetCloseExec(spec.dest.fd.qemu) < 0 || - virSetCloseExec(spec.dest.fd.local) < 0) { + virSecurityManagerSetImageFDLabel(driver->securityManager, vm, + spec.dest.fd.qemu) < 0) { virReportSystemError(errno, "%s", _("cannot create pipe for tunnelled migration")); goto cleanup;
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Aug 29, 2011 at 11:40:23AM -0600, Eric Blake wrote:
On 08/15/2011 01:58 AM, Jiri Denemark wrote:
By opening a connection to remote qemu process ourselves and passing the socket to qemu we get much better errors than just "migration failed" when the connection is opened by qemu. --- src/qemu/qemu_migration.c | 128 ++++++++++++++++++++++++++++++++++----------- 1 files changed, 98 insertions(+), 30 deletions(-)
+ if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)) { + int fds[2]; + + spec.destType = MIGRATION_DEST_FD; + spec.dest.fd.qemu = -1; + spec.dest.fd.local = -1; + + if (pipe(fds) == 0) { + spec.dest.fd.qemu = fds[1]; + spec.dest.fd.local = fds[0]; + } + if (spec.dest.fd.qemu == -1 || + virSetCloseExec(spec.dest.fd.qemu) < 0 || + virSetCloseExec(spec.dest.fd.local) < 0) { + virReportSystemError(errno, "%s", + _("cannot create pipe for tunnelled migration"));
SELinux doesn't like this. We never label the pipe here, and qemuMonitorMigrateToFd doesn't label the outgoing pipe either. Thus, when we hand the fd to qemu for tunneled migration, SELinux rejects the first write() attempt, and qemu fails with:
internal error unable to execute QEMU command 'getfd': No file descriptor supplied via SCM_RIGHTS
I'm still testing this, but based on how we label the incoming pipe in qemuProcessStart, I think this will solve the problem.
diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c index a2dc97c..38b05a9 100644 --- i/src/qemu/qemu_migration.c +++ w/src/qemu/qemu_migration.c @@ -24,6 +24,7 @@ #include <sys/time.h> #include <gnutls/gnutls.h> #include <gnutls/x509.h> +#include <fcntl.h>
#include "qemu_migration.h" #include "qemu_monitor.h" @@ -1691,13 +1692,13 @@ static int doTunnelMigrate(struct qemud_driver *driver, spec.dest.fd.qemu = -1; spec.dest.fd.local = -1;
- if (pipe(fds) == 0) { + if (pipe2(fds, O_CLOEXEC) == 0) { spec.dest.fd.qemu = fds[1]; spec.dest.fd.local = fds[0]; } if (spec.dest.fd.qemu == -1 || - virSetCloseExec(spec.dest.fd.qemu) < 0 || - virSetCloseExec(spec.dest.fd.local) < 0) { + virSecurityManagerSetImageFDLabel(driver->securityManager, vm, + spec.dest.fd.qemu) < 0) { virReportSystemError(errno, "%s", _("cannot create pipe for tunnelled migration")); goto cleanup;
Okay, I managed to reproduce the problem and this fixes it, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 08/30/2011 08:49 AM, Daniel Veillard wrote:
I'm still testing this, but based on how we label the incoming pipe in qemuProcessStart, I think this will solve the problem.
diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c index a2dc97c..38b05a9 100644 --- i/src/qemu/qemu_migration.c if (spec.dest.fd.qemu == -1 || - virSetCloseExec(spec.dest.fd.qemu)< 0 || - virSetCloseExec(spec.dest.fd.local)< 0) { + virSecurityManagerSetImageFDLabel(driver->securityManager, vm, + spec.dest.fd.qemu)< 0) { virReportSystemError(errno, "%s", _("cannot create pipe for tunnelled migration")); goto cleanup;
Okay, I managed to reproduce the problem and this fixes it,
ACK,
Thanks; pushed with this commit message: commit e6b8bc812af254f2ec6321b3cb7e9210b519deb0 Author: Eric Blake <eblake@redhat.com> Date: Mon Aug 29 17:31:42 2011 -0600 qemu: properly label outgoing pipe for tunneled migration Commit 3261761 made it possible to use pipes instead of sockets for outgoing tunneled migration; however, it caused a regression because the pipe was never given a SELinux label. * src/qemu/qemu_migration.c (doTunnelMigrate): Label outgoing pipe. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Aug 15, 2011 at 09:58:10 +0200, Jiri Denemark wrote:
Since qemu doesn't give us any reasonable errors when migration fails because of connection issues, we now create a connection to destination qemu ourselves and just pass the created socket to qemu.
Daniel P. Berrange (1): Add API for duplicating a socket/client file descriptor
Jiri Denemark (5): Add backlog parameter to virNetSocketListen Support changing UNIX socket owner in virNetSocketNewListenUNIX qemu: Refactor do{Tunnel,Native}Migrate functions qemu: Use virNetSocket for tunneled migration qemu: Use fd: protocol for migration
src/qemu/qemu_migration.c | 541 +++++++++++++++++++++-------------------- src/rpc/virnetclient.c | 20 ++ src/rpc/virnetclient.h | 3 + src/rpc/virnetserverservice.c | 6 +- src/rpc/virnetsocket.c | 29 ++- src/rpc/virnetsocket.h | 5 +- tests/virnetsockettest.c | 10 +- 7 files changed, 336 insertions(+), 278 deletions(-)
I modified the patches according to comments, added fcntl into boostrap.conf and pushed the series. Thanks for reviews. Jirka
participants (3)
-
Daniel Veillard -
Eric Blake -
Jiri Denemark