Re: [libvirt] [PATCH] Allow for URI aliases when connecting to libvirt

On 10/13/2011 04:53 AM, Daniel P. Berrange wrote: >From: "Daniel P. Berrange&quot;&lt;berrange(a)redhat.com&gt; > >I finally got fed up of typing URIs when using virsh.... > >This adds support for a libvirt client configuration file >either /etc/libvirt/libvirt.conf for privileged clients, >or $HOME/.libvirt/libvirt.conf for unprivileged clients. Cool idea! Potential problem - our testsuite uses -c test:///default (or similar); there's a case where we _don't_ want to use alias lookup. But I guess if valid alias names cannot contain ':', then the presence of ':' in the target name is sufficient to prove that we don't want to use aliases. [This is a review as I go reply, so I'll see what the code actually does...] >+<h2> >+<a name="URI_config">Configuring URI aliases</a> >+</h2> >+ >+<p> >+To simplify live for administrators, it is possible to setup URI aliases in a s/live/life/ >+libvirt client configuration file. The configuration file is<code>/etc/libvirt/libvirt.conf</code> >+for the root user, or<code>$HOME/.libvirt/libvirt.conf</code> for any unprivileged user. Really? Shouldn't it instead be that /etc is for qemu:///system, and $HOME/.libvirt is for any user, _including root_, for qemu:///session.

>+<p> >+ A URI alias should be a string made up from the characters >+<code>a-Z, 0-9, _, -</code>. Following the<code>=</code> >+ can be any libvirt URI string, including arbitrary URI parameters. >+ URI aliases will apply to any application opening a libvirt >+ connection, unless it has explicitly passed the<code>VIR_CONNECT_NO_ALIASES</code> >+ parameter to<code>virConnectOpenAuth</code>. Should we document that aliases are not consulted if the non-NULL connection name includes ':'? >+++ b/include/libvirt/libvirt.h.in >@@ -843,7 +843,8 @@ typedef virNodeMemoryStats *virNodeMemoryStatsPtr; > * Flags when opening a connection to a hypervisor > */ > typedef enum { >- VIR_CONNECT_RO = 1, /* A readonly connection */ >+ VIR_CONNECT_RO = (1<< 0), /* A readonly connection */ >+ VIR_CONNECT_NO_ALIASES = (1<< 1), /* Don't try to resolve URI aliases */ At first glance, I didn't see a use for this bit: it seems like the decision to use aliases is unambiguous - if the name contains ':', there is no alias, and if it lacks ':', then the only way it can succeed is if an alias lookup is successful. Oh, I see - you use VIR_CONNECT_NO_ALIASES to force failure rather than attempt an alias lookup for the case where name has no ':'. Okay, it makes sense.

>+ >+ entry = value->list; >+ while (entry) { >+ char *offset; >+ size_t safe; >+ >+ if (entry->type != VIR_CONF_STRING) { >+ virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s", Wouldn't VIR_ERR_CONF_SYNTAX go better here? >+ _("Expected a string for 'uri_aliases' config parameter list entry")); >+ return -1; >+ } >+ >+ if (!(offset = strchr(entry->str, '='))) { >+ virLibConnError(VIR_ERR_INTERNAL_ERROR, and here >+ _("Malformed 'uri_aliases' config entry '%s', expected 'alias=uri://host/path'"), >+ entry->str); >+ return -1; >+ } >+ >+ safe = strspn(entry->str, "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_-"); >+ if (safe< (offset - entry->str)) { >+ virLibConnError(VIR_ERR_INTERNAL_ERROR, and here