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@xxxxxxxxxx>
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@xxxxxxxxxx>
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
