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

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



On Fri, Nov 14, 2008 at 11:35:16AM +0000, Daniel P. Berrange wrote:
> On Fri, Nov 14, 2008 at 11:26:49AM +0100, Daniel Veillard wrote:
> > 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...
> 
> The reason I remove it, was because once we store the xmlUriPtr there
> was not actually any functional code which used the 'name' field. All
> the drivers' open methods will now potentially have to set 'uri', so
> I felt it easier to just remove 'name', and not have possibility of one
> driver forgetting to also change 'name' when changing the 'uri'.

  okay

> > > +    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.
> 
> Hmm, doesn't printf just turn NULL into the string '(null)' ? We kind

  Linux one yes, others no :-)

> of rely on that not crashing, more or less everywhere in libvirt.c
> for the DEBUG macros. Some of these are false positives, but the vast
> majority have potential for the %s argument to be NULL, since they're
> accepting input directly from the public API.
> 
> $ grep DEBUG src/libvirt.c | grep '%s'
>     DEBUG ("registering %s as driver %d",
>     DEBUG("libVir=%p, type=%s, typeVer=%p", libVer, type, typeVer);
>     DEBUG("Using LIBVIRT_DEFAULT_URI %s", defname);
>     DEBUG("Probed %s", latest);
>     DEBUG("Could not probe any hypervisor defaulting to %s",
>     DEBUG("Using %s as default URI, %d hypervisor found",
>     DEBUG("name \"%s\" to URI components:\n"
>     DEBUG("trying driver %d (%s) ...",
>     DEBUG("driver %d %s returned %s",
>     DEBUG("network driver %d %s returned %s",
>     DEBUG("storage driver %d %s returned %s",
>     DEBUG("name=%s", name);
>     DEBUG("name=%s", name);
>     DEBUG("name=%s, auth=%p, flags=%d", name, auth, flags);
>     DEBUG("conn=%p, type=%s", conn, type);
>     DEBUG("conn=%p, xmlDesc=%s, flags=%d", conn, xmlDesc, flags);
>     DEBUG("conn=%p, uuid=%s", conn, uuid);
>     DEBUG("conn=%p, uuidstr=%s", conn, uuidstr);
>     DEBUG("conn=%p, name=%s", conn, name);
>     DEBUG("domain=%p, to=%s", domain, to);
>     DEBUG("conn=%p, from=%s", conn, from);
>     DEBUG("domain=%p, to=%s, flags=%d", domain, to, flags);
>     DEBUG("domain=%p, dconn=%p, flags=%lu, dname=%s, uri=%s, bandwidth=%lu",
>     DEBUG("dconn=%p, cookie=%p, cookielen=%p, uri_in=%s, uri_out=%p, flags=%lu, dname=%s, bandwidth=%lu", dconn, cookie, cookielen, uri_in, uri_out, flags, dname, bandwidth);
>     DEBUG("domain=%p, cookie=%p, cookielen=%d, uri=%s, flags=%lu, dname=%s, bandwidth=%lu", domain, cookie, cookielen, uri, flags, dname, bandwidth);
>     DEBUG("dconn=%p, dname=%s, cookie=%p, cookielen=%d, uri=%s, flags=%lu", dconn, dname, cookie, cookielen, uri, flags);
>     DEBUG("domain=%p, path=%s, stats=%p, size=%zi", dom, path, stats, size);
>     DEBUG("domain=%p, path=%s, stats=%p, size=%zi", dom, path, stats, size);
>     DEBUG("domain=%p, path=%s, offset=%lld, size=%zi, buffer=%p",
>     DEBUG("conn=%p, xml=%s", conn, xml);
>     DEBUG("domain=%p, xml=%s", domain, xml);
>     DEBUG("domain=%p, xml=%s", domain, xml);
>     DEBUG("conn=%p, name=%s", conn, name);
>     DEBUG("conn=%p, uuid=%s", conn, uuid);
>     DEBUG("conn=%p, uuidstr=%s", conn, uuidstr);
>     DEBUG("conn=%p, xmlDesc=%s", conn, xmlDesc);
>     DEBUG("conn=%p, xml=%s", conn, xml);
>     DEBUG("conn=%p, name=%s", conn, name);
>     DEBUG("conn=%p, uuid=%s", conn, uuid);
>     DEBUG("conn=%p, uuidstr=%s", conn, uuidstr);
>     DEBUG("conn=%p, xmlDesc=%s", conn, xmlDesc);
>     DEBUG("conn=%p, xml=%s", conn, xml);
>     DEBUG("pool=%p, name=%s", pool, name);
>     DEBUG("conn=%p, key=%s", conn, key);
>     DEBUG("conn=%p, path=%s", conn, path);

  Let's keep as-is that can be handled separately

> 
> >   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 !
> 
> The auto-spawn & daemon stuff is the bit I tested most carefully in this
> patch, so hopefully no bad surprises there...

  Okidoc :-)

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]