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

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'.

> > + 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

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);

> 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...