On Mon, 2021-03-15 at 10:21 +0100, Michal Privoznik wrote:
> On 3/9/21 2:47 PM, Andrea Bolognani wrote:
> > +static int
> > +virProcessGetLimitFromProc(pid_t pid,
> > + int resource,
> > + struct rlimit *limit)
> > +{
> > + g_autofree char *procfile = NULL;
> > + g_autofree char *buf = NULL;
> > + g_auto(GStrv) lines = NULL;
> > + const char *label;
> > + size_t i;
> > +
> > + if (!(label = virProcessLimitResourceToLabel(resource))) {
> > + return -1;
> > + }
>
> While I am in favor of curly braces, this is incosistent with the rest
> of the function. However, you'll need to add another line here, probably
> so in the end you'll need to keep them. See [1].
>
> > @@ -768,6 +861,15 @@ virProcessGetLimit(pid_t pid,
> > if (virProcessPrLimit(pid, resource, NULL, old_limit) == 0)
> > return 0;
> >
> > + /* 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;
>
> There is just a single caller of this virProcessGetLimit() function and
> it expects errno to be set on failure. But that's not always the case
> with your patch. Moreover, I'd expect the !Linux stub to set ENOSYS.
Good catch!
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;
if (!(lines = g_strsplit(buf, "\n", 0)))
- return -1;
+ goto error;
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:
+ 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__) */
--
Andrea Bolognani / Red Hat / Virtualization
