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
