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@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@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