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

Re: [libvirt] [PATCH 05/12] Define public API for managing identities



On 05/02/2012 05:44 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 

A bit sparse on the commit message; even mentioning the term
virIdentityPtr would help future crawls through 'git log' find this patch.

> +++ b/include/libvirt/libvirt.h.in
> @@ -1088,6 +1088,36 @@ typedef virConnectAuth *virConnectAuthPtr;
>  
>  VIR_EXPORT_VAR virConnectAuthPtr virConnectAuthPtrDefault;
>  
> +typedef struct _virIdentity virIdentity;
> +typedef virIdentity *virIdentityPtr;
> +
> +typedef enum {
> +      VIR_IDENTITY_ATTR_UNIX_USER_NAME,
> +      VIR_IDENTITY_ATTR_UNIX_GROUP_NAME,
> +      VIR_IDENTITY_ATTR_UNIX_PROCESS_ID,
> +      VIR_IDENTITY_ATTR_SASL_USER_NAME,
> +      VIR_IDENTITY_ATTR_X509_DISTINGUISHED_NAME,
> +      VIR_IDENTITY_ATTR_SECURITY_CONTEXT,
> +
> +      VIR_IDENTITY_ATTR_LAST,

This last value should be guarded by VIR_ENUM_SENTINELS.

> +
> +virIdentityPtr virIdentityNew(void);
> +int virIdentityRef(virIdentityPtr ident);
> +int virIdentitySetAttr(virIdentityPtr ident,
> +                       unsigned int attr,
> +                       const char *value);
> +
> +int virIdentityGetAttr(virIdentityPtr ident,
> +                       unsigned int attr,
> +                       const char **value);
> +
> +int virIdentityIsEqual(virIdentityPtr identA,
> +                       virIdentityPtr identB);
> +
> +int virIdentityFree(virIdentityPtr ident);

Are there any other useful operations, such as knowing how many
attributes in the identity are currently set?

>  
>  
>  /**
> + * VIR_IDENTITY_MAGIC:
> + *
> + * magic value used to protect the API when pointers to identity structures
> + * are passed down by the users.
> + */
> +# define VIR_IDENTITY_MAGIC	0xB33FCAF3

How do we pick these magic numbers, anyway? :)

>  
> +
> +struct _virIdentity {
> +    unsigned int magic;
> +    virMutex lock;
> +    int refs;
> +
> +    char *attrs[VIR_IDENTITY_ATTR_LAST];
> +};

It looks like your intent is to store everything in this identity
locally (contrast it to _virDomain, which stores only enough information
locally to pass back over RPC to the remote side and have the remote
side do a hash lookup for the rest of the domain information).  It
should be okay, though, especially since this identity does not include
a name or uuid which could be used for a hash lookup for additional
contents, anyway.

> +
> +/**
> + * virIdentityNew:
> + *
> + * Creates a new empty identity object. After creating, one or
> + * more identifying attributes should be set on the identity.
> + *
> + * Returns: a new empty identity

or NULL on error.

> +/**
> + * virIdentityRef:
> + * @ident: the identity to hold a reference on
> + *
> + * Increment the reference count on the identity. For each
> + * additional call to this method, there shall be a corresponding
> + * call to virIdentityFree to release the reference count, once
> + * the caller no longer needs the reference to this object.
> + *
> + * This method is typically useful for applications where multiple
> + * threads are using an identity object, and it is required that
> + * the identity remain around until all threads have finished using
> + * it. ie, each new thread using a identity would increment

It looks weird to start a sentence with "ie,", but I don't have an
alternative wording on the tip of my tongue.

> + * the reference count.
> + *
> + * Returns 0 in case of success and -1 in case of failure.
> + */
> +int virIdentityRef(virIdentityPtr ident)
> +{
> +    if ((!VIR_IS_IDENTITY(ident))) {
> +        virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);

We really should be using a better error message than __FUNCTION__,
especially since virLibConnError is already adding __FUNCTION__ into the
list of arguments.  (Throughout the patch).

> +int virIdentitySetAttr(virIdentityPtr ident,
> +                       unsigned int attr,
> +                       const char *value)
> +{
> +    VIR_DEBUG("ident=%p attribute=%u value=%s", ident, attr, NULLSTR(value));
> +
> +    if ((!VIR_IS_IDENTITY(ident))) {
> +        virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
> +        virDispatchError(NULL);
> +        return -1;
> +    }
> +
> +    if (attr >= VIR_IDENTITY_ATTR_LAST) {
> +        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virDispatchError(NULL);
> +        return -1;
> +    }
> +
> +    if (!value) {
> +        virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__);

Really?  This means I can't clear out a value by passing in NULL.
Should there be a counterpart API for clearing an attribute?

> +        virDispatchError(NULL);
> +        return -1;
> +    }
> +
> +    virMutexLock(&ident->lock);
> +
> +    if (ident->attrs[attr]) {
> +        virLibConnError(VIR_ERR_OPERATION_DENIED, "%s",
> +                        _("Identity attribute is already set"));

Then again, this makes it a write-once interface (once an identity
attribute is set, you can't change it).  Should we document that better?


> +int virIdentityIsEqual(virIdentityPtr identA,
> +                       virIdentityPtr identB)

> +
> +    for (i = 0 ; i < VIR_IDENTITY_ATTR_LAST ; i++) {
> +        if (identA->attrs[i] == NULL &&
> +            identB->attrs[i] != NULL)
> +            goto cleanup;
> +        if (identA->attrs[i] != NULL &&
> +            identB->attrs[i] == NULL)
> +            goto cleanup;
> +        if (STRNEQ(identA->attrs[i],
> +                   identB->attrs[i]))
> +            goto cleanup;

Use STRNEQ_NULLABLE to shorten this.


> +++ b/src/libvirt_public.syms
> @@ -527,6 +527,11 @@ LIBVIRT_0.9.10 {
>          virDomainShutdownFlags;
>          virStorageVolResize;
>          virStorageVolWipePattern;
> +        virIdentityNew;
> +        virIdentityIsEqual;
> +        virIdentitySetAttr;
> +        virIdentityGetAttr;
> +        virIdentityFree;
>  } LIBVIRT_0.9.9;

Wrong release.  This should be in the LIBVIRT_0.9.12 section.


> +++ b/src/util/virterror.c
> @@ -1259,6 +1259,12 @@ virErrorMsg(virErrorNumber error, const char *info)
>              else
>                  errmsg = _("block copy still active: %s");
>              break;
> +        case VIR_ERR_INVALID_IDENTITY:
> +            if (info == NULL)
> +                errmsg = _("invalid identity pointer in");

That reads awkwardly.  That says if I call:

reportError(VIR_ERR_INVALID_IDENTITY, NULL);

my error message will be "function: invalid identity pointer in".

> +            else
> +                errmsg = _("invalid identity pointer in %s");

Then again, if I call

reportError(VIR_ERR_INVALID_IDENTITY, __FUNCTION__);

the error message will be "function: invalid identity pointer in function"

But it's copy and paste from existing practice, so it's no worse than
before.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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