[libvirt] [PATCH v3 46/48] util: storage identity attrs as virTypedParameter internally

[Christophe de Dinechin]

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)