Re: [libvirt] [PATCH v2 2/6] Define internal APIs for managing identities

On Wed, Mar 13, 2013 at 15:24:01 +0000, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > 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.