Re: [libvirt] [PATCH] Allow overriding default URI in config file

On 03/14/2012 06:37 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > 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?

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

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