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