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

[Eric Blake]

On 03/14/2012 06:37 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at 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?

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

Probably worth a v2 to fix the problems.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120314/f327e8b5/attachment-0001.sig>