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

[Jiri Denemark]

On Wed, Mar 13, 2013 at 15:24:01 +0000, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at 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