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

Re: [libvirt] PATCH: 4/12: Refactoring URI probing



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 veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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