[libvirt PATCH v2 1/4] util: Try to get limits from /proc

[Michal Privoznik]

On 3/15/21 12:22 PM, Andrea Bolognani wrote:
> On Mon, 2021-03-15 at 11:57 +0100, Andrea Bolognani wrote:
>> The changes you're suggesting are not trivial enough for me to feel
>> comfortable simply applying them locally and then pushing right away.
>> Would the diff below look reasonable squashed in?
>>
>> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
>> index 4fa854090d..cae0dabae3 100644
>> --- a/src/util/virprocess.c
>> +++ b/src/util/virprocess.c
>> @@ -799,16 +799,17 @@ virProcessGetLimitFromProc(pid_t pid,
>>       size_t i;
>>
>>       if (!(label = virProcessLimitResourceToLabel(resource))) {
>> +        virReportSystemError(EINVAL, "%s", _("Invalid resource"));
>>           return -1;
>>       }
>>
>>       procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid);
>>
>>       if (virFileReadAllQuiet(procfile, 2048, &buf) < 0)
>> -        return -1;
>> +        goto error;
>>
> [...]
>>
>> + error:
>> +    virReportSystemError(EIO, "%s", _("Input/output error"));
>> +    return -1;
>>   }
>>   # else /* !defined(__linux__) */
>>   static int
>> @@ -846,6 +851,7 @@ virProcessGetLimitFromProc(pid_t pid G_GNUC_UNUSED,
>>                              int resource G_GNUC_UNUSED,
>>                              struct rlimit *limit G_GNUC_UNUSED)
>>   {
>> +    virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
>>       return -1;
>>   }
>>   # endif /* !defined(__linux__) */
> 
> This probably makes more sense:
> 
> diff --git a/src/util/virprocess.c b/src/util/virprocess.c
> index 4fa854090d..b173856b7a 100644
> --- a/src/util/virprocess.c
> +++ b/src/util/virprocess.c
> @@ -799,16 +799,17 @@ virProcessGetLimitFromProc(pid_t pid,
>       size_t i;
> 
>       if (!(label = virProcessLimitResourceToLabel(resource))) {
> +        errno = EINVAL;
>           return -1;
>       }
> 
>       procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid);
> 
>       if (virFileReadAllQuiet(procfile, 2048, &buf) < 0)
> -        return -1;
> +        goto error;

virFileReadAllQuiet() sets errno, this would just overwrite it with 
something less specific.

> 
>       if (!(lines = g_strsplit(buf, "\n", 0)))
> -        return -1;
> +        goto error;

I think this check can be dropped. g_strsplit() doesn't ever return NULL 
really, does it?

> 
>       for (i = 0; lines[i]; i++) {
>           g_autofree char *softLimit = NULL;
> @@ -820,25 +821,29 @@ virProcessGetLimitFromProc(pid_t pid,
>               continue;
>   
>           if (sscanf(line, "%ms %ms %*s", &softLimit, &hardLimit) < 2)
> -            return -1;
> +            goto error;
>   
>           if (STREQ(softLimit, "unlimited")) {
>               limit->rlim_cur = RLIM_INFINITY;
>           } else {
>               if (virStrToLong_ull(softLimit, NULL, 10, &tmp) < 0)
> -                return -1;
> +                goto error;
>               limit->rlim_cur = tmp;
>           }
>           if (STREQ(hardLimit, "unlimited")) {
>               limit->rlim_max = RLIM_INFINITY;
>           } else {
>               if (virStrToLong_ull(hardLimit, NULL, 10, &tmp) < 0)
> -                return -1;
> +                goto error;
>               limit->rlim_max = tmp;
>           }
>       }
>   
>       return 0;
> +
> + error:
> +    errno = EIO;
> +    return -1;
>   }
>   # else /* !defined(__linux__) */
>   static int
> @@ -846,6 +851,7 @@ virProcessGetLimitFromProc(pid_t pid G_GNUC_UNUSED,
>                              int resource G_GNUC_UNUSED,
>                              struct rlimit *limit G_GNUC_UNUSED)
>   {
> +    errno = ENOSYS;
>       return -1;
>   }
>   # endif /* !defined(__linux__) */
> 

The rest looks good.

Michal