On 04/25/2013 11:57 AM, Laine Stump wrote:
> This patch adds two sets of functions:
>
> 1) lower level functions that will immediately set the
> RLIMIT_MEMLOCK. RLIMIT_NPROC, or RLIMIT_NOFILE of either the current
> process (using setrlimit()) or any other process (using prlimit()).
>
> 2) functions for virCommand* that will setup a virCommand object to
> set those limits at a later time just after it has forked a new
> process, but before it execs the new program.
>
> configure.ac has prlimit and setrlimit added to the list of functions
> to check for, and the low level functions are hopefully properly
> "unsupported" error) on all platforms.
>
> You'll notice some ugly typecasting around pid_t; this is all an
> attempt to follow the formula given to me by Eric for getting it to
> compile without warnings on all platforms.
> ---
> configure.ac | 2 +-
> src/libvirt_private.syms | 6 ++
> src/util/vircommand.c | 38 ++++++++++++
> src/util/vircommand.h | 4 ++
> src/util/virutil.c | 146 +++++++++++++++++++++++++++++++++++++++++++++++
> src/util/virutil.h | 4 ++
> 6 files changed, 199 insertions(+), 1 deletion(-)
>
> @@ -598,6 +602,13 @@ virExec(virCommandPtr cmd)
> goto fork_error;
> }
>
> + if (virSetMaxMemLock(-1, cmd->maxMemLock) < 0)
> + goto fork_error;
> + if (virSetMaxProcesses(-1, cmd->maxProcesses) < 0)
> + goto fork_error;
> + if (virSetMaxFiles(-1, cmd->maxFiles) < 0)
> + goto fork_error;
Hmm. uid_t and gid_t can validly be 0, hence the sentinel has to be -1.
But pid_t can never be 0 (it starts with 1), so it is easier if you let
0 be the sentinel that means current process.
> +#if HAVE_SETRLIMIT
> +
> +# if HAVE_PRLIMIT
> +static int
> +virPrLimit(pid_t pid, int resource, struct rlimit *rlim)
> +{
> + return prlimit(pid, resource, rlim, NULL);
This doesn't report errors;
> +}
> +# else
> +static int
> +virPrLimit(pid_t pid ATTRIBUTE_UNUSED,
> + int resource ATTRIBUTE_UNUSED,
> + struct rlimit *rlim ATTRIBUTE_UNUSED)
> +{
> + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
> + return -1;
while this does. Either the caller should always be responsible for
reporting the errors, or you should fix the HAVE_PRLIMIT version to call
virReportSystemError() as needed. Looking below at your callers, I go
for the latter - simplify the non-prlimit variant to just be:
errno = ENOSYS;
return -1;
[1]...
> +}
> +# endif
> +
> +int
> +virSetMaxMemLock(pid_t pid, unsigned long long bytes)
> +{
> + struct rlimit rlim;
> +
> + if (bytes == 0)
> + return 0;
> +
> + rlim.rlim_cur = rlim.rlim_max = bytes;
> + if (pid == (pid_t)-1) {
Overkill; since there is never a process id of 0, that makes a sentinel
that is much easier to check for. (You did get the cast right, per our
IRC conversation; only I didn't realize why you needed the cast in the
first place - pid_t -1 is the process id that represents the entire
process group of the init(1) process).
> + if (setrlimit(RLIMIT_MEMLOCK, &rlim) < 0) {
RLIMIT_MEMLOCK is not portable. POSIX lists only the following:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/setrlimit.html
RLIMIT_CORE
RLIMIT_CPU
RLIMIT_DATA
RLIMIT_FSIZE
RLIMIT_NOFILE
RLIMIT_STACK
RLIMIT_AS
The rest are extensions, so you'll need to wrap this code in #ifdef
RLIMIT_MEMLOCK and provide a fallback when it is not present. You need
#ifdef instead of #if (POSIX guarantees that all RLIMIT_* extensions in
<sys/resource.h> will be macros, but does not guarantee which, if any,
of those macros will have the value 0), but at least you don't need to
touch configure.ac for this particular probe.
> + virReportSystemError(errno,
> + _("cannot limit locked memory to %llu"),
> + bytes);
> + return -1;
> + }
> + } else {
> + if (virPrLimit(pid, RLIMIT_MEMLOCK, &rlim) < 0) {
prlimit(0, ...) is identical to prlimit(getpid(), ...), which in turn is
identical to setrlimit(...). Then again, since setrlimit() is more
portable than prlimit, I agree with you doing the differentiation on the
sentinel pid yourself instead of blindly trying virPrLimit().
> + virReportSystemError(errno,
> + _("cannot limit locked memory "
> + "of process %lld to %llu"),
> + (long long int)pid, bytes);
> + return -1;
...[1] since here we always report any failure.
> + }
> + }
> + return 0;
> +}
> +
> +int
> +virSetMaxProcesses(pid_t pid, unsigned int procs)
> +{
> + struct rlimit rlim;
> +
> + if (procs == 0)
> + return 0;
> +
> + rlim.rlim_cur = rlim.rlim_max = procs;
> + if (pid == (pid_t)-1) {
> + if (setrlimit(RLIMIT_NPROC, &rlim) < 0) {
Another non-portable limit, needing #ifdef treatment.
...
> + rlim.rlim_cur = rlim.rlim_max = files + 1;
> + if (pid == (pid_t)-1) {
> + if (setrlimit(RLIMIT_NOFILE, &rlim) < 0) {
POSIX requires this one. You can play brave and blindly use it (since
we already guarded things by an outer HAVE_SETRLIMIT (about the only
platform where we might have problems would be cygwin, which doesn't yet
provide all of the posix-mandated limits; but I don't have my cygwin
machine booted at the moment to check which limits it lacks), or you
could play it consistent and do the same thing here as for the other two
extension limits. At any rate, I'll do a cygwin build after we freeze
for 1.0.5 but before the final release, to clean up any mess we may have
made in our assumptions here.
> +#else
> +int
> +virSetMaxMemLock(pid_t pid ATTRIBUTE_UNUSED, unsigned long long bytes)
> +{
> + if (bytes == 0)
> + return 0;
> +
> + virReportSystemError(ENOSYS, "%s", _("Not supported on this platform"));
> + return -1;
Nice that you let the call ignore unchanged settings, and only error for
impossible-to-fulfill limits.
> +++ b/src/util/virutil.h
> @@ -53,6 +53,10 @@ int virSetCloseExec(int fd) ATTRIBUTE_RETURN_CHECK;
> int virPipeReadUntilEOF(int outfd, int errfd,
> char **outbuf, char **errbuf);
>
> +int virSetMaxMemLock(pid_t pid, unsigned long long bytes);
> +int virSetMaxProcesses(pid_t pid, unsigned int procs);
> +int virSetMaxFiles(pid_t pid, unsigned int files);
And of course fix Dan's comments about putting these in the right file.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list