[libvirt PATCH 12/17] util: Try to get limits from /proc

[Daniel P. Berrangé]

On Mon, Mar 08, 2021 at 04:21:16PM +0100, Andrea Bolognani wrote:
> On Mon, 2021-03-08 at 10:50 +0000, Daniel P. Berrangé wrote:
> > On Fri, Mar 05, 2021 at 08:13:59PM +0100, Andrea Bolognani wrote:
> > > +    if (!(label = virProcessLimitResourceToLabel(resource))) {
> > > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > > +                       _("Unknown resource %d requested for process %lld"),
> > > +                       resource, (long long)pid);
> > > +        return -1;
> > 
> > Setting errors on -1
> 
> This is only hit if virProcessGetLimitFromProc() has been asked to
> obtain limits for a resource it doesn't know how to fetch, which
> indicates a bug in libvirt and is thus reported as internal error.

The our coding standard is that we only call virReportError if the
caller is actually going to honour the errors.

> 
> > > +    procfile = g_strdup_printf("/proc/%lld/limits", (long long)pid);
> > > +
> > > +    if (!g_file_get_contents(procfile, &buf, &len, NULL))
> > > +        return -1;
> > 
> > Not setting errors on -1
> 
> This is simply "file couldn't be read", which would be the case on
> FreeBSD for example.

Right, but we *must* always be consistent in whether a method uses
virReportError or not.  We have two code paths here returning -1,
and only one of which reports errors. Either all must report
errors, or none. Since the only caller is ignoring errors, then
it looks like none should report errors.

> 
> > > +    /* For whatever reason, using prlimit() on another process - even
> > > +     * when it's just to obtain the current limit rather than changing
> > > +     * it - requires CAP_SYS_RESOURCE, which we might not have in a
> > > +     * containerized environment; on the other hand, no particular
> > > +     * permission is needed to poke around /proc, so try that if going
> > > +     * through the syscall didn't work */
> > > +    if (virProcessGetLimitFromProc(pid, resource, old_limit) == 0)
> > > +        return 0;
> > 
> > This ought to be conditional for Linux only and error reporting needs
> > to be made consistent.
> 
> The intent above was to have this fail quietly on non-Linux without
> adding checks for it, but sure I can have an actual stub on other
> platforms instead.
> 
> -- 
> Andrea Bolognani / Red Hat / Virtualization
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|