On Thu, Nov 13, 2008 at 05:25:53PM +0000, Daniel P. Berrange wrote:
> This patch changes the way hypervisor URIs are probed when NULL is passed
> to the virConnectOpen() method. Previously we had an explicit probe method
> called in each driver. This does not work when we move some drivers out of
> libvirt.so and into libvirtd daemon. So I've refactored the way this works
> so that we simply pass a NULL uri into the individual driver open methods.
> If they see a NULL uri, they will make an attempt to open, and return a
> VIR_DRV_OPEN_DECLINED code if its not suitable. This also works for the
> remote driver, probing supported URI inside the daemon. Quite alot of code
> churn but the end result should be functionally the same
Okay, makes sense,
[...]
> +++ b/src/datatypes.c Wed Nov 12 21:59:20 2008 +0000
> @@ -174,7 +174,7 @@
> */
> static void
> virReleaseConnect(virConnectPtr conn) {
> - DEBUG("release connection %p %s", conn, conn->name);
> + DEBUG("release connection %p", conn);
maybe conn->name should have been kept to help with debug,
each time conn->uri was set it should be easy to keep name too.
not a blocker, just a suggestion...
> if (conn->domains != NULL)
> virHashFree(conn->domains, (virHashDeallocator) virDomainFreeName);
> if (conn->networks != NULL)
> @@ -188,7 +188,7 @@
> if (virLastErr.conn == conn)
> virLastErr.conn = NULL;
>
> - VIR_FREE(conn->name);
> + xmlFreeURI(conn->uri);
>
> pthread_mutex_unlock(&conn->lock);
> pthread_mutex_destroy(&conn->lock);
> @@ -213,7 +213,7 @@
> return(-1);
> }
> pthread_mutex_lock(&conn->lock);
> - DEBUG("unref connection %p %s %d", conn, conn->name, conn->refs);
> + DEBUG("unref connection %p %d", conn, conn->refs);
> conn->refs--;
> refs = conn->refs;
> if (refs == 0) {
> @@ -322,7 +322,7 @@
> VIR_FREE(domain->name);
> VIR_FREE(domain);
>
> - DEBUG("unref connection %p %s %d", conn, conn->name, conn->refs);
> + DEBUG("unref connection %p %d", conn, conn->refs);
> conn->refs--;
> if (conn->refs == 0) {
> virReleaseConnect(conn);
> @@ -458,7 +458,7 @@
> VIR_FREE(network->name);
> VIR_FREE(network);
>
> - DEBUG("unref connection %p %s %d", conn, conn->name, conn->refs);
> + DEBUG("unref connection %p %d", conn, conn->refs);
> conn->refs--;
> if (conn->refs == 0) {
> virReleaseConnect(conn);
> @@ -591,7 +591,7 @@
> VIR_FREE(pool->name);
> VIR_FREE(pool);
>
> - DEBUG("unref connection %p %s %d", conn, conn->name, conn->refs);
> + DEBUG("unref connection %p %d", conn, conn->refs);
> conn->refs--;
> if (conn->refs == 0) {
> virReleaseConnect(conn);
> @@ -729,7 +729,7 @@
> VIR_FREE(vol->pool);
> VIR_FREE(vol);
>
> - DEBUG("unref connection %p %s %d", conn, conn->name, conn->refs);
> + DEBUG("unref connection %p %d", conn, conn->refs);
> conn->refs--;
> if (conn->refs == 0) {
> virReleaseConnect(conn);
> diff -r d97fa69e255b src/datatypes.h
> --- a/src/datatypes.h Wed Nov 12 21:05:13 2008 +0000
> +++ b/src/datatypes.h Wed Nov 12 21:59:20 2008 +0000
> @@ -87,7 +87,7 @@
> struct _virConnect {
> unsigned int magic; /* specific value to check */
> int flags; /* a set of connection flags */
> - char *name; /* connection URI */
> + xmlURIPtr uri; /* connection URI */
>
> /* The underlying hypervisor driver and network driver. */
> virDriverPtr driver;
> diff -r d97fa69e255b src/driver.h
> --- a/src/driver.h Wed Nov 12 21:05:13 2008 +0000
> +++ b/src/driver.h Wed Nov 12 21:59:20 2008 +0000
> @@ -63,11 +63,8 @@
> #define VIR_DRV_SUPPORTS_FEATURE(drv,conn,feature) \
> ((drv)->supports_feature ? (drv)->supports_feature((conn),(feature)) : 0)
>
> -typedef const char *
> - (*virDrvProbe) (void);
> typedef virDrvOpenStatus
> (*virDrvOpen) (virConnectPtr conn,
> - xmlURIPtr uri,
> virConnectAuthPtr auth,
> int flags);
> typedef int
> @@ -302,7 +299,6 @@
> int no; /* the number virDrvNo */
> const char * name; /* the name of the driver */
> unsigned long ver; /* the version of the backend */
> - virDrvProbe probe;
> virDrvOpen open;
> virDrvClose close;
> virDrvDrvSupportsFeature supports_feature;
> diff -r d97fa69e255b src/libvirt.c
> --- a/src/libvirt.c Wed Nov 12 21:05:13 2008 +0000
> +++ b/src/libvirt.c Wed Nov 12 21:59:20 2008 +0000
> @@ -685,8 +685,11 @@
> int flags)
> {
> int i, res;
> - virConnectPtr ret = NULL;
> - xmlURIPtr uri;
> + virConnectPtr ret;
> +
> + ret = virGetConnect();
> + if (ret == NULL)
> + return NULL;
>
> /*
> * If no URI is passed, then check for an environment string if not
> @@ -699,74 +702,43 @@
> DEBUG("Using LIBVIRT_DEFAULT_URI %s", defname);
> name = defname;
> } else {
> - const char *use = NULL;
> - const char *latest;
> - int probes = 0;
> - for (i = 0; i < virDriverTabCount; i++) {
> - if ((virDriverTab[i]->probe != NULL) &&
> - ((latest = virDriverTab[i]->probe()) != NULL)) {
> - probes++;
> -
> - DEBUG("Probed %s", latest);
> - /*
> - * if running a xen kernel, give it priority over
> - * QEmu emulation
> - */
> - if (STREQ(latest, "xen:///"))
> - use = latest;
> - else if (use == NULL)
> - use = latest;
> - }
> - }
> - if (use == NULL) {
> - name = "xen:///";
> - DEBUG("Could not probe any hypervisor defaulting to %s",
> - name);
> - } else {
> - name = use;
> - DEBUG("Using %s as default URI, %d hypervisor found",
> - use, probes);
> - }
> - }
> - }
> -
> - /* Convert xen -> xen:/// for back compat */
> - if (STRCASEEQ(name, "xen"))
> - name = "xen:///";
> -
> - /* Convert xen:// -> xen:/// because xmlParseURI cannot parse the
> - * former. This allows URIs such as xen://localhost to work.
> - */
> - if (STREQ (name, "xen://"))
> - name = "xen:///";
> -
> - ret = virGetConnect();
> - if (ret == NULL)
> - return NULL;
> -
> - uri = xmlParseURI (name);
> - if (!uri) {
> - virLibConnError (ret, VIR_ERR_INVALID_ARG,
> - _("could not parse connection URI"));
> - goto failed;
> - }
> -
> - DEBUG("name \"%s\" to URI components:\n"
> - " scheme %s\n"
> - " opaque %s\n"
> - " authority %s\n"
> - " server %s\n"
> - " user %s\n"
> - " port %d\n"
> - " path %s\n",
> - name,
> - uri->scheme, uri->opaque, uri->authority, uri->server,
> - uri->user, uri->port, uri->path);
> -
> - ret->name = strdup (name);
> - if (!ret->name) {
> - virLibConnError (ret, VIR_ERR_NO_MEMORY, _("allocating conn->name"));
> - goto failed;
> + name = NULL;
> + }
> + }
> +
> + if (name) {
> + /* Convert xen -> xen:/// for back compat */
> + if (STRCASEEQ(name, "xen"))
> + name = "xen:///";
> +
> + /* Convert xen:// -> xen:/// because xmlParseURI cannot parse the
> + * former. This allows URIs such as xen://localhost to work.
> + */
> + if (STREQ (name, "xen://"))
> + name = "xen:///";
> +
> + ret->uri = xmlParseURI (name);
> + if (!ret->uri) {
> + virLibConnError (ret, VIR_ERR_INVALID_ARG,
> + _("could not parse connection URI"));
> + goto failed;
> + }
> +
> + DEBUG("name \"%s\" to URI components:\n"
> + " scheme %s\n"
> + " opaque %s\n"
> + " authority %s\n"
> + " server %s\n"
> + " user %s\n"
> + " port %d\n"
> + " path %s\n",
> + name,
> + ret->uri->scheme, ret->uri->opaque,
> + ret->uri->authority, ret->uri->server,
> + ret->uri->user, ret->uri->port,
> + ret->uri->path);
Hum... that could crash on some OSes, many of those strings might get
NULL, actually opaque will be NULL if you have path.
....
Okay, the remote extension and the auto-spawn of a user daemon for
testing makes it a more complex than expected fix, but I don't see any
simpler way. Only testing will tell us if something is missing
compatibility wise, so let's push it and wait for feedback !
+1
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list