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