Andrea Bolognani writes:
On Mon, 2019-07-29 at 18:11 +0100, Daniel P. Berrangé wrote:
util: storage identity attrs as virTypedParameter internally
- 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.
Thanks ;-)
[...]
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.
Curious why (I'm too lazy to look it up in earlier discussions)? In general, giving a name to an int type is a good idea, isn't it?
- *pid = (pid_t)val; + if (ret == 1) + *pid = (gid_t)val;
This should be
*pid = (pid_t)val;
You made me look at that code again, and now I'm confused as to why it's OK to leave garbage in *pid if we fail to find the corresponding typed param. Previously, the function returned -1 in that case, to indicate failure. Now, it returns 0, but does not update *uid. Is that intentional?
[...]
+++ 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
-- Cheers, Christophe de Dinechin (IRC c3d)