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

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" <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? 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 :-) > >> + 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