Re: [libvirt] [PATCH 1/4] Fix UUID handling in secrets/storage encryption APIs

11 Sep 2009


      On Fri, Sep 11, 2009 at 03:19:17PM +0100, Daniel P. Berrange wrote:
...
Convert all the secret/storage encryption APIs / wire format to
handle UUIDs in raw format instead of non-canonical printable
format. Guarentees data format correctness.
[...]
+++ b/include/libvirt/libvirt.h
@@ -1467,12 +1467,17 @@ int                     virConnectNumOfSecrets  (virConnectPtr conn);
 int                     virConnectListSecrets   (virConnectPtr conn,
                                                  char **uuids,
                                                  int maxuuids);
+virSecretPtr            virSecretLookupByUUID(virConnectPtr conn,
+                                              const unsigned char *uuid);
 virSecretPtr            virSecretLookupByUUIDString(virConnectPtr conn,
                                                     const char *uuid);
 virSecretPtr            virSecretDefineXML      (virConnectPtr conn,
                                                  const char *xml,
                                                  unsigned int flags);
-char *                  virSecretGetUUIDString  (virSecretPtr secret);
+int                     virSecretGetUUID        (virSecretPtr secret,
+                                                 unsigned char *buf);
+int                     virSecretGetUUIDString  (virSecretPtr secret,
+                                                 char *buf);
 char *                  virSecretGetXMLDesc     (virSecretPtr secret,
                                                  unsigned int flags);
 int                     virSecretSetValue       (virSecretPtr secret,
explicit ACK, this is better !

[..]
...
@@ -4828,7 +4828,7 @@ get_nonnull_storage_vol (virConnectPtr conn, remote_nonnull_storage_vol vol)
 static virSecretPtr
 get_nonnull_secret (virConnectPtr conn, remote_nonnull_secret secret)
 {
-    return virGetSecret (conn, secret.uuid);
+    return virGetSecret (conn, BAD_CAST secret.uuid);
heh I didn't expect BAD_CAST to be used outside libxml2 :-)

[...]
...
@@ -1179,7 +1181,9 @@ virGetSecret(virConnectPtr conn, const char *uuid)
     }
     virMutexLock(&conn->lock);
-    ret = virHashLookup(conn->secrets, uuid);
+    virUUIDFormat(uuid, uuidstr);
+
+    ret = virHashLookup(conn->secrets, uuidstr);
Would you mind also removing the 

  Returns 0 in case of success and -1 in case of error.

comment for virUUIDFormat() as it's void and doesn't do any checking
(it has no way to).

[...]
...
/**
- * virSecretLookupByUUIDString:
- * @conn: virConnect connection
- * @uuid: ID of a secret
+ * virSecretLookupByUUID:
+ * @conn: pointer to the hypervisor connection
+ * @uuid: the raw UUID for the secret
  *
- * Fetches a secret based on uuid.
+ * Try to lookup a secret on the given hypervisor based on its UUID.
+ * uses the 16 bytes of raw data to describe the UUID
...
*
- * Returns the secret on success, or NULL on failure.
+ * Returns a new secret object or NULL in case of failure.  If the
+ * secret cannot be found, then VIR_ERR_NO_SECRET error is raised.
  */
 virSecretPtr
-virSecretLookupByUUIDString(virConnectPtr conn, const char *uuid)
+virSecretLookupByUUID(virConnectPtr conn, const unsigned char *uuid)
[...]
/**
+ * virSecretLookupByUUIDString:
+ * @conn: pointer to the hypervisor connection
+ * @uuidstr: the string UUID for the secret
+ *
+ * Try to lookup a secret on the given hypervisor based on its UUID.
+ * Uses the printable string value to describe the UUID
...
+ *
+ * Returns a new secret object or NULL in case of failure.  If the
+ * secret cannot be found, then VIR_ERR_NO_SECRET error is raised.
+ */
+virSecretPtr
+virSecretLookupByUUIDString(virConnectPtr conn, const char *uuidstr)
+{
+    int raw[VIR_UUID_BUFLEN], i;
+    unsigned char uuid[VIR_UUID_BUFLEN];
+    int ret;
+
+    DEBUG("conn=%p, uuidstr=%s", conn, uuidstr);
+
+    virResetLastError();
+
+    if (!VIR_IS_CONNECT(conn)) {
+        virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
+        return (NULL);
+    }
+    if (uuidstr == NULL) {
+        virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
+        goto error;
+    }
+    /* XXX: sexpr_uuid() also supports 'xxxx-xxxx-xxxx-xxxx' format.
+     *      We needn't it here. Right?
+     */
Hum, I would rather use virUUIDParse() to make sure there is no
confusion, a public API should be tolerant about this I think
and it's cleaner to reuse it !
...
+    ret = sscanf(uuidstr,
+                 "%02x%02x%02x%02x-"
+                 "%02x%02x-"
+                 "%02x%02x-"
+                 "%02x%02x-"
+                 "%02x%02x%02x%02x%02x%02x",
+                 raw + 0, raw + 1, raw + 2, raw + 3,
+                 raw + 4, raw + 5, raw + 6, raw + 7,
+                 raw + 8, raw + 9, raw + 10, raw + 11,
+                 raw + 12, raw + 13, raw + 14, raw + 15);
+
+    if (ret!=VIR_UUID_BUFLEN) {
+        virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
+        goto error;
+    }
+    for (i = 0; i < VIR_UUID_BUFLEN; i++)
+        uuid[i] = raw[i] & 0xFF;
ACK, overall an important cleanup, as it affects the API and the
  network format !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

    