On Wed, Mar 13, 2013 at 15:24:01 +0000, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Introduce a local object virIdentity for managing security attributes used to form a client application's identity. Instances of this object are intended to be used as if they were immutable, once created & populated with attributes
...
diff --git a/src/util/virerror.c b/src/util/virerror.c index 40c3b25..8cb8548 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -118,6 +118,7 @@ VIR_ENUM_IMPL(virErrorDomain, VIR_ERR_DOMAIN_LAST, "SSH transport layer", /* 50 */ "Lock Space", "Init control", + "Identity", )
@@ -1213,6 +1214,12 @@ virErrorMsg(virErrorNumber error, const char *info) else errmsg = _("resource busy %s"); break; + case VIR_ERR_INVALID_IDENTITY: + if (info == NULL) + errmsg = _("invalid identity"); + else + errmsg = _("invalid identity %s"); + break;
This doesn't seem to be used anywhere yet but would "invalid identity: %s" result in better error messages?
} return errmsg; } diff --git a/src/util/viridentity.c b/src/util/viridentity.c new file mode 100644 index 0000000..7ebf851 --- /dev/null +++ b/src/util/viridentity.c
...
+/** + * virIdentityIsEqual: + * @identA: the first identity + * @identB: the second identity + * + * Compares every attribute in @identA and @identB + * to determine if they refer to the same identity + * + * Returns 1 if they are equal, 0 if not equal or -1 on error + */ +int virIdentityIsEqual(virIdentityPtr identA, + virIdentityPtr identB) +{ + int ret = 0; + size_t i; + VIR_DEBUG("identA=%p identB=%p", identA, identB); + + for (i = 0 ; i < VIR_IDENTITY_ATTR_LAST ; i++) { + if (STRNEQ_NULLABLE(identA->attrs[i], + identB->attrs[i])) + goto cleanup; + } + + ret = 1; +cleanup: + return ret; +}
This API never fails, so why do we need to document unreachable -1 on error and use int rather than bool? Especially when you use this API as if it was just returning true/false.
diff --git a/tests/viridentitytest.c b/tests/viridentitytest.c new file mode 100644 index 0000000..adb4f7d --- /dev/null +++ b/tests/viridentitytest.c ... +static int testIdentityAttrs(const void *data ATTRIBUTE_UNUSED) +{ + int ret = -1; + virIdentityPtr ident; + const char *val; + + if (!(ident = virIdentityNew())) + goto cleanup; + + if (virIdentitySetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_USER_NAME, + "fred") < 0) + goto cleanup; + + if (virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_USER_NAME, + &val) < 0) + goto cleanup; + + if (STRNEQ_NULLABLE(val, "fred")) { + VIR_DEBUG("Expected 'fred' got '%s'", NULLSTR(val)); + goto cleanup; + } + + if (virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_GROUP_NAME, + &val) < 0) + goto cleanup; + + if (STRNEQ_NULLABLE(val, NULL)) {
Hmm, could be easier to just check if val is NULL :-)
+ VIR_DEBUG("Unexpected groupname attribute"); + goto cleanup; + } + + if (virIdentitySetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_USER_NAME, + "joe") != -1) { + VIR_DEBUG("Unexpectedly overwrote attribute"); + goto cleanup; + } + + if (virIdentityGetAttr(ident, + VIR_IDENTITY_ATTR_UNIX_USER_NAME, + &val) < 0) + goto cleanup; + + if (STRNEQ_NULLABLE(val, "fred")) { + VIR_DEBUG("Expected 'fred' got '%s'", NULLSTR(val)); + goto cleanup; + } + + ret = 0; +cleanup: + virObjectUnref(ident); + return ret; +} + + +static int testIdentityEqual(const void *data ATTRIBUTE_UNUSED) +{ + int ret = -1; + virIdentityPtr identa = NULL; + virIdentityPtr identb = NULL; + + if (!(identa = virIdentityNew())) + goto cleanup; + if (!(identb = virIdentityNew())) + goto cleanup; + + if (!virIdentityIsEqual(identa, identb)) { + VIR_DEBUG("Empty identities were no equal");
s/no/not/
+ goto cleanup; + } ...
ACK with the comments addressed. Jirka