On Wed, Mar 14, 2012 at 06:54:40AM -0600, Eric Blake wrote: > On 03/14/2012 06:37 AM, Daniel P. Berrange wrote: > > From: "Daniel P. Berrange" <berra...@redhat.com> > > > > Currently if the URI passed to virConnectOpen* is NULL, then we > > > > - Look for LIBVIRT_DEFAULT_URI env var > > - Probe for drivers > > > > This changes it so that > > > > - Look for LIBVIRT_DEFAULT_URI env var > > - Look for 'uri_default' in $HOME/.libvirt/libvirt.conf > > - Probe for drivers > > Nice. Do we need any followup patches for virsh? That is, where should > VIRSH_DEFAULT_CONNECT_URI fit into the hierarchy?
We should deprecate it :-) > > > docs/uri.html.in | 13 +++++++ > > src/libvirt.c | 107 > > +++++++++++++++++++++++++++++++++++++----------------- > > 2 files changed, 87 insertions(+), 33 deletions(-) > > Where is the change to src/libvirt.conf to document this feature? Missing :-) > > +++ b/docs/uri.html.in > > @@ -52,6 +52,19 @@ uri_aliases = [ > > set, no alias lookup will be attempted. > > </p> > > > > + <h2><a name="URI_default">Default URI choice</a></h2> > > + > > + <p> > > +If the URI passed to <code>virConnectOpen*</code> is NULL, then libvirt > > will use the following > > +logic to determine what URI to use. > > +</p> > > + > > + <ol> > > + <li>The environment variable <code>LIBVIRT_DEFAULT_URI</code></li> > > + <li>The client configuration file <code>uri_default</code> > > parameter</li> > > + <li>Probe each hypervisor in turn until one that works is found</li> > > + </ol> > > + > > Looks good. The location of the client configuration file was mentioned > earlier on the same page. > > > <h2> > > <a name="URI_virsh">Specifying URIs to virsh, virt-manager and > > virt-install</a> > > Maybe this section should mention that without any -c option and without > VIRSH_DEFAULT_CONNECT_URI, then virsh passes NULL to the virConnectOpen, > which then falls back on the above hierarchy for libvirt in general. > > > > > > +static int virConnectGetConfigFile(virConfPtr *conf) > > Formatting nit - should this be two lines? > > > > + > > +static int virConnectGetDefaultURI(virConfPtr conf, > > And this one? > > > + const char **name) > > +{ > > + int ret = -1; > > + virConfValuePtr value = NULL; > > + char *defname = getenv("LIBVIRT_DEFAULT_URI"); > > + if (defname && *defname) { > > + VIR_DEBUG("Using LIBVIRT_DEFAULT_URI %s", defname); > > + *name = defname; > > Needs a 'ret = 0; goto cleanup;' here, otherwise... > > > + } > > + > > + if ((value = virConfGetValue(conf, "uri_default"))) { > > + if (value->type != VIR_CONF_STRING) { > > + virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", > > + _("Expected a string for 'uri_default' config > > parameter")); > > + goto cleanup; > > + } > > + > > + *name = value->str; > > this assignment is a memory leak if both the envvar and config file are > present. We're returning const strings here, so there's no leak to deal with Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list