Re: [libvirt] [PATCH 2/6] process: Add virProcessGetMaxMemLock()

On Wed, 2015-12-09 at 12:00 -0500, John Ferlan wrote: > On 11/24/2015 08:56 AM, Andrea Bolognani wrote: >> @@ -788,6 +788,48 @@ virProcessSetMaxMemLock(pid_t pid ATTRIBUTE_UNUSED, unsigned long long bytes) >> } >> #endif /* ! (HAVE_SETRLIMIT && defined(RLIMIT_MEMLOCK)) */ >> >> +#if HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK) >> +int >> +virProcessGetMaxMemLock(pid_t pid, >> + unsigned long long *bytes) > > Another option would be to return bytes... > > where at least as I read patch 3 bytes == 0 is no different than > original_memlock == 0 especially if arg2 is NULL > > Of course, does calling getrlimit or virProcessPrLimit() potentially > multiple times make sense? Does blasting those error messages each time > to the log, but continuing on cause confusion? > > I guess what I'm thinking about is how it's eventually used in patch 3 > and the concept of failure because something in here fails or perhaps > the getrlimit *or* prlimit isn't supported on the platform... I think ignoring failures in getting the current memory locking limit is the right thing to do in the upper layers, as that just means we won't be able to restore the original limit afterwards and that's not really a huge problem. However, at this level, we're dealing with low-level functionality and I think all failures should be reported appropriately, eg. with a negative return value and a proper error logged. >> +{ >> + struct rlimit rlim; >> + >> + if (!bytes) >> + return 0; > > Since you return 0 here if passed a NULL bytes, then I think you'd have > to do so in the other API What other API do you mean?

>> + >> + if (pid == 0) { >> + if (getrlimit(RLIMIT_MEMLOCK, &rlim) < 0) { >> + virReportSystemError(errno, >> + "%s", > > This one could go on previous line - no big deal other than extra line I'd rather squeeze the second and third lines together or leave it as it is, if that's the same to you.

>> + _("cannot get locked memory limit")); >> + return -1; >> + } >> + } else { >> + if (virProcessPrLimit(pid, RLIMIT_MEMLOCK, NULL, &rlim) < 0) { >> + virReportSystemError(errno, >> + _("cannot get locked memory limit " >> + "of process %lld"), >> + (long long int) pid); >> + return -1; >> + } >> + } >> + >> + /* virProcessSetMaxMemLock() sets both rlim_cur and rlim_max to the >> + * same value, so we can retrieve just rlim_max here */ >> + *bytes = rlim.rlim_max; > > One oddball thought... what if rlim.rlim_max == 0? Is that possible? I > suppose only if rlim_cur == 0 too, right? Not sure it makes sense and I > didn't chase the syscall. It does say rlim_cur can be 0 and that > rlim_max must be equal or higher... > > I'm just trying to logically follow through on the thought of how 0 is > used in patch 3. I don't know, but I don't think we should concern ourself too much with that case as virProcessSetMaxMemLock(), which will be used to restore whatever value this function returns, ignores the value 0.

>> + >> + return 0; >> +} >> +#else /* ! (HAVE_GETRLIMIT && defined(RLIMIT_MEMLOCK)) */ >> +int >> +virProcessGetMaxMemLock(pid_t pid ATTRIBUTE_UNUSED, >> + unsigned long long *bytes) > > Would technically be unused if kept as is... However since the other API > returns 0 when value is passed as NULL, this could too. Having a function that either fails as unimplemented or succeeds depending on its arguments doesn't feel right to me. I see, however, that all virProcessSetMax*() functions behave this way already so it makes sense to change do the same for consistency's sake.