[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] [RFC, PATCH] Re: xen:/// vs. xen://FQDN/ vs xen+unix:/// discrepancy



Hello,

Am Freitag 23 Juli 2010 18:02:27 schrieb Philipp Hahn:
> Am Donnerstag 22 Juli 2010 14:51:22 schrieb Daniel P. Berrange:
> > > root xen4# virsh -c xen://xen4.domain.name/ list
> > > root xen4# virsh -c xen:/// list
> > > root xen4# virsh -c xen+unix:/// list
...
> To me it looks like some connection between libvirtd / virsh and xen gets
> broken, so no data on running domains can be retrieved any more, until I
> restart libvirtd. After that everything is back to normal until the next
> hickup.

Today I encountered the bug again: "virsh list" was not showing any domain, 
while "virsh -c xen+unix:///" did show "Domain 0" and my other domain.

I attached gdb, "thread apply all bt" showed no thread still executing 
xenHypervisorInit(), but I found the following inconsistent state:
> in_init == 1
> hypervisor_version == -1
> sys_interface_version == -1
> dom_interface_version == -1

Notice in_init=1: Currently there are two error path, where that can happen:
1. ret = open(XEN_HYPERVISOR_SOCKET, O_RDWR);
2. if (VIR_ALLOC(ipt) < 0) {
In both cases in_init remains 1, which disables further useful logging:

> #define virXenError(code, ...)                                             \
> if (in_init == 0)                                                  \ 
> virReportErrorHelper(NULL, VIR_FROM_XEN, code, __FILE__,       \
> __FUNCTION__, __LINE__, __VA_ARGS__)
 
This leads me to the following question:
1. Why is error reporting deactivated during init?
Since in_init==1 is valid only during the call xenHypervisorInit(), explicitly 
using that macro there for logging seems very wrong.

2. xenHypervisorInit() also allocates three regexp_t structures, which are 
only freed when one of them can not be compiled, but they remain in memory 
when determin the Xen hypervisor version fails.
On the other hand these 3 structures are used in 
xenHypervisorMakeCapabilitiesInternal(). This isn't checked directly, but 
only indirectly, because in the error case no one will ever call it with a 
valid virConnectPtr.
Should the structures be freed in all error cases?

The attached patch is a first try at improving that situation.

> > To verify that libvirtd is actually openening the same drivers as virsh
> > you can edit /etc/libvirt/libvirtd.conf and set
> >
> >   log_filters="1:libvirt 1:xen"
> >   log_outputs="1:file:/var/log/libvirt/libvirtd.log"
>
> I've now enabled that and see if I can gather more data.

This didn't work because of the bug described above.

Sincerely
Philipp Hahn
-- 
Philipp Hahn           Open Source Software Engineer      hahn univention de
Univention GmbH        Linux for Your Business        fon: +49 421 22 232- 0
Mary-Somerville-Str.1  D-28359 Bremen                 fax: +49 421 22 232-99
                                                   http://www.univention.de/
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c
index 9cb8e8c..06b7281 100644
--- a/src/xen/xen_hypervisor.c
+++ b/src/xen/xen_hypervisor.c
@@ -1943,7 +1943,7 @@ xenHypervisorInit(void)
         char error[100];
         regerror (errcode, &flags_hvm_rec, error, sizeof error);
         regfree (&flags_hvm_rec);
-        virXenError(VIR_ERR_INTERNAL_ERROR, "%s", error);
+        VIR_ERROR0(error);
         in_init = 0;
         return -1;
     }
@@ -1953,7 +1953,7 @@ xenHypervisorInit(void)
         regerror (errcode, &flags_pae_rec, error, sizeof error);
         regfree (&flags_pae_rec);
         regfree (&flags_hvm_rec);
-        virXenError(VIR_ERR_INTERNAL_ERROR, "%s", error);
+        VIR_ERROR0(error);
         in_init = 0;
         return -1;
     }
@@ -1964,7 +1964,7 @@ xenHypervisorInit(void)
         regfree (&xen_cap_rec);
         regfree (&flags_pae_rec);
         regfree (&flags_hvm_rec);
-        virXenError(VIR_ERR_INTERNAL_ERROR, "%s", error);
+        VIR_ERROR0(error);
         in_init = 0;
         return -1;
     }
@@ -1972,8 +1972,11 @@ xenHypervisorInit(void)
     /* Xen hypervisor version detection begins. */
     ret = open(XEN_HYPERVISOR_SOCKET, O_RDWR);
     if (ret < 0) {
+        virReportSystemError(errno,
+                             _("cannot open file %s"),
+                             XEN_HYPERVISOR_SOCKET);
         hypervisor_version = -1;
-        return(-1);
+        goto failed1;
     }
     fd = ret;
 
@@ -2018,11 +2021,9 @@ xenHypervisorInit(void)
      */
 
     hypervisor_version = -1;
-    virXenError(VIR_ERR_XEN_CALL, " ioctl %lu",
+	VIR_ERROR(" ioctl %lu",
                 (unsigned long) IOCTL_PRIVCMD_HYPERCALL);
-    VIR_FORCE_CLOSE(fd);
-    in_init = 0;
-    return(-1);
+    goto failed2;
 
  detect_v2:
     /*
@@ -2034,7 +2035,7 @@ xenHypervisorInit(void)
 
     if (VIR_ALLOC(ipt) < 0) {
         virReportOOMError();
-        return(-1);
+        goto failed2;
     }
     /* Currently consider RHEL5.0 Fedora7, xen-3.1, and xen-unstable */
     sys_interface_version = 2; /* XEN_SYSCTL_INTERFACE_VERSION */
@@ -2104,17 +2105,19 @@ xenHypervisorInit(void)
 
     VIR_DEBUG0("Failed to find any Xen hypervisor method");
     hypervisor_version = -1;
-    virXenError(VIR_ERR_XEN_CALL, " ioctl %lu",
+	VIR_ERROR(" ioctl %lu",
                 (unsigned long)IOCTL_PRIVCMD_HYPERCALL);
+    VIR_FREE(ipt);
+ failed2:
     VIR_FORCE_CLOSE(fd);
+ failed1:
     in_init = 0;
-    VIR_FREE(ipt);
     return(-1);
 
  done:
     VIR_FORCE_CLOSE(fd);
-    in_init = 0;
     VIR_FREE(ipt);
+    in_init = 0;
     return(0);
 }
 

Attachment: signature.asc
Description: This is a digitally signed message part.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]