Re: [libvirt] [PATCH] Better error reporting for failed migration.

2010-02-19 Thread Daniel Veillard 
On Thu, Feb 18, 2010 at 10:38:37AM -0500, Chris Lalancette wrote:
 If the hostname as returned by gethostname resolves
 to localhost (as it does with the broken Fedora-12
 installer), then live migration will fail because the
 source will try to migrate to itself.  Detect this
 situation up-front and abort the live migration before
 we do any real work.
 
 Signed-off-by: Chris Lalancette clala...@redhat.com
 ---
  src/libvirt_private.syms |1 +
  src/qemu/qemu_driver.c   |2 +-
  src/util/util.c  |   37 +++--
  src/util/util.h  |1 +
  4 files changed, 38 insertions(+), 3 deletions(-)
 
 diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
 index e3806cd..69ad686 100644
 --- a/src/libvirt_private.syms
 +++ b/src/libvirt_private.syms
 @@ -568,6 +568,7 @@ virExecDaemonize;
  virSetCloseExec;
  virSetNonBlock;
  virFormatMacAddr;
 +virGetHostnameLocalhost;
  virGetHostname;
  virParseMacAddr;
  virFileDeletePid;
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index 0d8ec04..2123880 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -7748,7 +7748,7 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn,
  if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0;
  
  /* Get hostname */
 -if ((hostname = virGetHostname(dconn)) == NULL)
 +if ((hostname = virGetHostnameLocalhost(0)) == NULL)
  goto cleanup;
  
  /* XXX this really should have been a properly well-formed
 diff --git a/src/util/util.c b/src/util/util.c
 index cdab300..72cc222 100644
 --- a/src/util/util.c
 +++ b/src/util/util.c
 @@ -2193,11 +2193,11 @@ char *virIndexToDiskName(int idx, const char *prefix)
  #define AI_CANONIDN 0
  #endif
  
 -char *virGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED)
 +char *virGetHostnameLocalhost(int allow_localhost)
  {
  int r;
  char hostname[HOST_NAME_MAX+1], *result;
 -struct addrinfo hints, *info;
 +struct addrinfo hints, *info, *res;
  
  r = gethostname (hostname, sizeof(hostname));
  if (r == -1) {
 @@ -2217,6 +2217,34 @@ char *virGetHostname(virConnectPtr conn 
 ATTRIBUTE_UNUSED)
   hostname, gai_strerror(r));
  return NULL;
  }
 +
 +/* if we aren't allowing localhost, then we iterate through the
 + * list and make sure none of the IPv4 addresses are 127.0.0.1 and
 + * that none of the IPv6 addresses are ::1
 + */
 +if (!allow_localhost) {
 +res = info;
 +while (res) {
 +if (res-ai_family == AF_INET) {
 +if (htonl(((struct sockaddr_in 
 *)res-ai_addr)-sin_addr.s_addr) == INADDR_LOOPBACK) {
 +virUtilError(VIR_ERR_INTERNAL_ERROR, %s,
 + _(canonical hostname pointed to localhost, 
 but this is not allowed));
 +freeaddrinfo(info);
 +return NULL;
 +}
 +}
 +else if (res-ai_family == AF_INET6) {
 +if (IN6_IS_ADDR_LOOPBACK(((struct sockaddr_in6 
 *)res-ai_addr)-sin6_addr)) {
 +virUtilError(VIR_ERR_INTERNAL_ERROR, %s,
 + _(canonical hostname pointed to localhost, 
 but this is not allowed));
 +freeaddrinfo(info);
 +return NULL;
 +}
 +}
 +res = res-ai_next;
 +}
 +}
 +
  if (info-ai_canonname == NULL) {
  virUtilError(VIR_ERR_INTERNAL_ERROR,
   %s, _(could not determine canonical host name));
 @@ -2233,6 +2261,11 @@ char *virGetHostname(virConnectPtr conn 
 ATTRIBUTE_UNUSED)
  return result;
  }
  
 +char *virGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED)
 +{
 +return virGetHostnameLocalhost(1);
 +}
 +
  /* send signal to a single process */
  int virKillProcess(pid_t pid, int sig)
  {
 diff --git a/src/util/util.h b/src/util/util.h
 index 4207508..d024fe1 100644
 --- a/src/util/util.h
 +++ b/src/util/util.h
 @@ -232,6 +232,7 @@ static inline int getuid (void) { return 0; }
  static inline int getgid (void) { return 0; }
  #endif
  
 +char *virGetHostnameLocalhost(int allow_localhost);
  char *virGetHostname(virConnectPtr conn);
  
  int virKillProcess(pid_t pid, int sig);

  Yes that looks fine, ACK

thanks !

Daniel

-- 
Daniel Veillard  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
dan...@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] Better error reporting for failed migration.

2010-02-18 Thread Chris Lalancette 
If the hostname as returned by gethostname resolves
to localhost (as it does with the broken Fedora-12
installer), then live migration will fail because the
source will try to migrate to itself.  Detect this
situation up-front and abort the live migration before
we do any real work.

Signed-off-by: Chris Lalancette clala...@redhat.com
---
 src/libvirt_private.syms |1 +
 src/qemu/qemu_driver.c   |2 +-
 src/util/util.c  |   37 +++--
 src/util/util.h  |1 +
 4 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e3806cd..69ad686 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -568,6 +568,7 @@ virExecDaemonize;
 virSetCloseExec;
 virSetNonBlock;
 virFormatMacAddr;
+virGetHostnameLocalhost;
 virGetHostname;
 virParseMacAddr;
 virFileDeletePid;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0d8ec04..2123880 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7748,7 +7748,7 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn,
 if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0;
 
 /* Get hostname */
-if ((hostname = virGetHostname(dconn)) == NULL)
+if ((hostname = virGetHostnameLocalhost(0)) == NULL)
 goto cleanup;
 
 /* XXX this really should have been a properly well-formed
diff --git a/src/util/util.c b/src/util/util.c
index cdab300..72cc222 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -2193,11 +2193,11 @@ char *virIndexToDiskName(int idx, const char *prefix)
 #define AI_CANONIDN 0
 #endif
 
-char *virGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED)
+char *virGetHostnameLocalhost(int allow_localhost)
 {
 int r;
 char hostname[HOST_NAME_MAX+1], *result;
-struct addrinfo hints, *info;
+struct addrinfo hints, *info, *res;
 
 r = gethostname (hostname, sizeof(hostname));
 if (r == -1) {
@@ -2217,6 +2217,34 @@ char *virGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED)
  hostname, gai_strerror(r));
 return NULL;
 }
+
+/* if we aren't allowing localhost, then we iterate through the
+ * list and make sure none of the IPv4 addresses are 127.0.0.1 and
+ * that none of the IPv6 addresses are ::1
+ */
+if (!allow_localhost) {
+res = info;
+while (res) {
+if (res-ai_family == AF_INET) {
+if (htonl(((struct sockaddr_in 
*)res-ai_addr)-sin_addr.s_addr) == INADDR_LOOPBACK) {
+virUtilError(VIR_ERR_INTERNAL_ERROR, %s,
+ _(canonical hostname pointed to localhost, 
but this is not allowed));
+freeaddrinfo(info);
+return NULL;
+}
+}
+else if (res-ai_family == AF_INET6) {
+if (IN6_IS_ADDR_LOOPBACK(((struct sockaddr_in6 
*)res-ai_addr)-sin6_addr)) {
+virUtilError(VIR_ERR_INTERNAL_ERROR, %s,
+ _(canonical hostname pointed to localhost, 
but this is not allowed));
+freeaddrinfo(info);
+return NULL;
+}
+}
+res = res-ai_next;
+}
+}
+
 if (info-ai_canonname == NULL) {
 virUtilError(VIR_ERR_INTERNAL_ERROR,
  %s, _(could not determine canonical host name));
@@ -2233,6 +2261,11 @@ char *virGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED)
 return result;
 }
 
+char *virGetHostname(virConnectPtr conn ATTRIBUTE_UNUSED)
+{
+return virGetHostnameLocalhost(1);
+}
+
 /* send signal to a single process */
 int virKillProcess(pid_t pid, int sig)
 {
diff --git a/src/util/util.h b/src/util/util.h
index 4207508..d024fe1 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -232,6 +232,7 @@ static inline int getuid (void) { return 0; }
 static inline int getgid (void) { return 0; }
 #endif
 
+char *virGetHostnameLocalhost(int allow_localhost);
 char *virGetHostname(virConnectPtr conn);
 
 int virKillProcess(pid_t pid, int sig);
-- 
1.6.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Better error reporting for failed migration.

2009-10-19 Thread Chris Lalancette 
Daniel P. Berrange wrote:
 +if (STREQ(hostname, localhost)) {
 +VIR_FREE(hostname);
 +qemudReportError(dconn, NULL, NULL, VIR_ERR_INVALID_ARG, %s,
 + _(Could not resolve destination hostname; 
 +   either fix destination to resolve hostname, 
 
 +   or use the optional URI migration 
 parameter));
 +goto cleanup;
 +}
 +
 
 I think I'd be inclined to actually resolve the hostname, and then
 check it agaist  127.0.0.1  and ::1. You can get quite a few variations
 which ultimtely might point to localhost.

I did some testing here.  Basically I wrote a small program that just calls
getaddrinfo() like virHostname() does, iterates over all of the addresses
returned, and prints various information.

On my F-11 box, due to a possibly glibc bug, it only returns IPv4 addresses, and
the fields look like:

flags 0x82, family 2, socktype 1, protocol 6, addrlen 16, addr 0x1521b80,
canonname intel2.xmen.org
flags 0x82, family 2, socktype 2, protocol 17, addrlen 16, addr 0x1521bd0,
canonname (null)
flags 0x82, family 2, socktype 3, protocol 0, addrlen 16, addr 0x1521c20,
canonname (null)

On my F-12 box, which is the one that originally showed the problems, it shows
both IPv6 and IPv4 addresses, and the output looks like:

flags 0x82, family 10, socktype 1, protocol 6, addrlen 28, addr 0x19ad220,
canonname localhost
flags 0x82, family 10, socktype 2, protocol 17, addrlen 28, addr 0x19ad280,
canonname (null)
flags 0x82, family 10, socktype 3, protocol 0, addrlen 28, addr 0x19ad2e0,
canonname (null)
flags 0x82, family 2, socktype 1, protocol 6, addrlen 16, addr 0x19ad110,
canonname (null)
flags 0x82, family 2, socktype 2, protocol 17, addrlen 16, addr 0x19ad180,
canonname (null)
flags 0x82, family 2, socktype 3, protocol 0, addrlen 16, addr 0x19ad1d0,
canonname (null)

So it seems like the canonname resolves to localhost in both the IPv4 (family
2) and IPv6 (family 10) cases.  I guess we could pass the result of
virGetHostname() back into getaddrinfo() and look at the results, but I don't
know that it would be significantly different from what we already get.  Do you
still think it is worthwhile?

-- 
Chris Lalancette

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] Better error reporting for failed migration.

2009-10-16 Thread Daniel P. Berrange 
On Thu, Oct 15, 2009 at 11:26:38AM +0200, Chris Lalancette wrote:
 The comment in the code says most of it, but when the destination
 hostname resolution is screwed up, print a proper error instead
 of the very unhelpful unknown error.
 
 Note that I'm not overly fond of the wording in the error message,
 so I'm open to suggestions.
 
 Signed-off-by: Chris Lalancette clala...@redhat.com
 ---
  src/qemu/qemu_driver.c |   16 
  1 files changed, 16 insertions(+), 0 deletions(-)
 
 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
 index d37b184..02bb5cb 100644
 --- a/src/qemu/qemu_driver.c
 +++ b/src/qemu/qemu_driver.c
 @@ -6288,6 +6288,22 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn,
  goto cleanup;
  }
  
 +/* Remember that we are running on the destination.  The hostname 
 that
 + * we resolve here will be used on the source machine in the 
 migrate
 + * monitor command.  Because of that, localhost is almost always the
 + * wrong thing.  Adding this check explicitly breaks localhost
 + * migration, but only for those machines that have improperly
 + * configured hostname resolution.
 + */

NB, localhost migration has never worked, nor do we intend it to. You
will certainly deadlock libvirtd if you tried it with tunnelled migration
We should in fact try to protect against this in the 'perform' method.
After opening the libvirtd connection to the dest, it shoud call the
virGetHotsname(dconn) and compare it to virGetHostname(conn) and reject
it if it is the same


 +if (STREQ(hostname, localhost)) {
 +VIR_FREE(hostname);
 +qemudReportError(dconn, NULL, NULL, VIR_ERR_INVALID_ARG, %s,
 + _(Could not resolve destination hostname; 
 +   either fix destination to resolve hostname, 
 +   or use the optional URI migration 
 parameter));
 +goto cleanup;
 +}
 +

I think I'd be inclined to actually resolve the hostname, and then
check it agaist  127.0.0.1  and ::1. You can get quite a few variations
which ultimtely might point to localhost.



Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] Better error reporting for failed migration.

2009-10-15 Thread Chris Lalancette 
The comment in the code says most of it, but when the destination
hostname resolution is screwed up, print a proper error instead
of the very unhelpful unknown error.

Note that I'm not overly fond of the wording in the error message,
so I'm open to suggestions.

Signed-off-by: Chris Lalancette clala...@redhat.com
---
 src/qemu/qemu_driver.c |   16 
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index d37b184..02bb5cb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6288,6 +6288,22 @@ qemudDomainMigratePrepare2 (virConnectPtr dconn,
 goto cleanup;
 }
 
+/* Remember that we are running on the destination.  The hostname that
+ * we resolve here will be used on the source machine in the migrate
+ * monitor command.  Because of that, localhost is almost always the
+ * wrong thing.  Adding this check explicitly breaks localhost
+ * migration, but only for those machines that have improperly
+ * configured hostname resolution.
+ */
+if (STREQ(hostname, localhost)) {
+VIR_FREE(hostname);
+qemudReportError(dconn, NULL, NULL, VIR_ERR_INVALID_ARG, %s,
+ _(Could not resolve destination hostname; 
+   either fix destination to resolve hostname, 
+   or use the optional URI migration parameter));
+goto cleanup;
+}
+
 /* XXX this really should have been a properly well-formed
  * URI, but we can't add in tcp:// now without breaking
  * compatability with old targets. We at least make the
-- 
1.6.0.6

--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list