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