On 05/02/2012 05:44 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list