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

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

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