[libvirt] [PATCH v3 46/48] util: storage identity attrs as virTypedParameter internally

[Andrea Bolognani]

On Mon, 2019-07-29 at 18:11 +0100, Daniel P. Berrangé wrote:
> util: storage identity attrs as virTypedParameter internally

s/storage/store/

[...]
> +++ b/src/util/viridentity.c
> @@ -188,6 +176,7 @@ virIdentityPtr virIdentityGetSystem(void)
>                                   _("Unable to lookup SELinux process context"));
>              return ret;
>          }
> +        VIR_DEBUG("Set con %s", con);

This looks like a leftover from development.

[...]
> -/**
> - * 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 true if they are equal, false if not equal
> - */
> -bool virIdentityIsEqual(virIdentityPtr identA,
> -                        virIdentityPtr identB)

This function was introduced with

  commit 3aabe27247711324df2bfa623e9a5e8d2442e3a5
  Author: Daniel P. Berrange <berrange at redhat.com>
  Date:   Fri Jan 20 17:49:32 2012 +0000

    Define internal APIs for managing identities

    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

    Signed-off-by: Daniel P. Berrange <berrange at redhat.com>

and apparently never used. Please drop it in a separate commit.

[...]
>  int virIdentityGetOSUserName(virIdentityPtr ident,
>                               const char **username)
>  {
> -    return virIdentityGetAttr(ident,
> -                              VIR_IDENTITY_ATTR_OS_USER_NAME,
> -                              username);

You forgot to do

  *username = NULL;

here.

[...]
>  int virIdentityGetOSUserID(virIdentityPtr ident,
>                             uid_t *uid)
>  {
> -    int val;
> -    const char *userid;
> +    unsigned long long val;
> +    int ret;

Usually we use 'ret' to store the return value of the current
function, and 'rc' for the return value of any sub-function we might
need to call. Please use 'rc' here to avoid any confusion.

> -    if (virStrToLong_i(userid, NULL, 10, &val) < 0)
> +    ret = virTypedParamsGetULLong(ident->params,
> +                                  ident->nparams,
> +                                  VIR_CONNECT_IDENTITY_OS_USER_ID,
> +                                  &val);
> +    if (ret < 0)
>          return -1;
>  
> -    *uid = (uid_t)val;
> +    if (ret == 1)
> +        *uid = (uid_t)val;

In case Christophe is following along: this is one of the cases where
libvirt functions don't follow the usual 0 means success, <0 means
failure mantra.

[...]
>  int virIdentityGetOSProcessID(virIdentityPtr ident,
>                                pid_t *pid)
>  {
> -    unsigned long long val;
> -    const char *processid;
> +    int ret;
> +    long long val;

I still think we should be using ull for pids.

> -    *pid = (pid_t)val;
> +    if (ret == 1)
> +        *pid = (gid_t)val;

This should be

  *pid = (pid_t)val;

[...]
> +++ b/tests/viridentitytest.c
> @@ -166,7 +109,8 @@ static int testIdentityGetSystem(const void *data)
>          goto cleanup;
>  
>      if (STRNEQ_NULLABLE(val, context)) {
> -        VIR_DEBUG("Unexpected SELinux context attribute");
> +        VIR_DEBUG("Unexpected SELinux context attribute '%s' != '%s'",
> +                  val, context);
>          goto cleanup;
>      }

This change also doesn't belong in this patch. You can put it in the
same one as the other SELinux-related test suite fix, though.

And since you're tweaking the message, I suggest something like

  VIR_DEBUG("Unexpected SELinux context: expected='%s' actual='%s'",
            context, val);

for easier debugging.

-- 
Andrea Bolognani / Red Hat / Virtualization