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

[Daniel Veillard]

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 at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/