Re: [libvirt] [PATCH v3 46/48] util: storage identity attrs as virTypedParameter internally
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(a)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(a)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