[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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"<berrange redhat com>

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

+<a name="URI_config">Configuring URI aliases</a>
+To simplify live for administrators, it is possible to setup URI aliases in a


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

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


+static char *
+    char *path;
+    if (geteuid() == 0) {
+        if (virAsprintf(&path, "%s/libvirt/libvirt.conf",
+                        SYSCONFDIR)<  0)
+            goto no_memory;

Hmm, while I asked above whether qemu:///session for root should use ~root/.libvirt, the code matches your documentation. So if you like my idea, then this needs a tweak (probably pass in a parameter for whether the connection itself is system vs. session).

+static int
+virConnectOpenFindURIAliasMatch(virConfValuePtr value, const char *alias, char **uri)
+    virConfValuePtr entry;
+    if (value->type != VIR_CONF_LIST) {
+        virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",

Fair enough - only a coding error would trigger this error.

+                        _("Expected a list for 'uri_aliases' config parameter"));
+        return -1;
+    }
+    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

+static int
+virConnectOpenResolveURIAlias(const char *alias, char **uri)
+    char *config = NULL;
+    int ret = -1;
+    virConfPtr conf = NULL;
+    virConfValuePtr value = NULL;
+    *uri = NULL;
+    /* Short circuit to avoid doing URI alias resolution
+     * when it clearly isn't an alias */
+    if (strchr(alias, '/') ||
+        strchr(alias, ':'))
+        return 0;

Aha, I was right - no alias lookup on full URIs.

diff --git a/src/libvirt.conf b/src/libvirt.conf
new file mode 100644
index 0000000..ffe8c21
--- /dev/null
+++ b/src/libvirt.conf
@@ -0,0 +1,13 @@

Why a leading blank line?

ACK with nits fixed.

Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]