Debian Bug report logs - #302079
s-s-d: Support to set ulimit

Reply or subscribe to this bug.

Display info messages

Message #5 received at submit@bugs.debian.org (full text, mbox, reply):

From: Carlo Contavalli <ccontavalli@debian.org>

To: Debian Bug Tracking System <submit@bugs.debian.org>

Subject: dpkg: [PATCH] setting ulimit from start-stop-daemon

Date: Wed, 30 Mar 2005 03:05:58 +0200

[Message part 1 (text/plain, inline)]

Package: dpkg
Version: 1.10.27
Severity: wishlist


Hello,
  with the attached patch, start-stop-daemon will look for
a ``limit'' file before loading a daemon, and set the ulimits
for the daemon accordingly. Just an example. Let's say 
we have an init script that launches:

   start-stop-daemon --start --exec /usr/sbin/adaemon -- $ARGS

With the attacched patch, start-stop-daemon will then look for a 
file called ``/etc/limits/adaemon'' containing something like:

  # Ok, limit core file sizes
core    soft    2048
core    hard    4096
nofile  soft    100
nofile  hard    200
#nproc   soft    50
#nproc   hard    150
cpu     soft    12
cpu     hard    15
data    soft    120000
data    hard    135000
#fsize   soft    14000
#fsize   hard    15000
rss     soft    10200
rss     hard    14500
stack   soft    120000
stack   hard    130000
memlock soft    15000
memlock hard    17000
as      soft    10000000
as      hard    10000000

  The patch should work quite fine. It respects the quiet and 
verbose flags, prints a warning in case of wrong lines or badly
formatted file, supports comments, and goes on as far as possible
in case of errors, ignores the absence of the file and should 
generally behave correctly in most conditions. I believe applying 
this patch would maintain complete backward compatibility, and only 
add useful functions. It adds ``transparent'' support for setting
ulimits on any daemon launched by start-stop-daemon.

  The patch should include support for *BSD systems, even if I 
haven't had the chance to test it on such systems.
I'm a DD too, and if there is any chance for it to be included
in the ``upstream'' dpkg, I'd be willing to work on writing
documentation, verify it works under kfreebsd and hurd, update
it for your current development tree and generally verify its
correct behavior.

  I believe it to be very important to have a system wide
method to set per-daemon limits, considering the always
existing problems about resource exaustion and oom.

  I don't like the resulting code pretty much, but should
be quite reliable (I put lot of attention on it), backward
compatible and generally compliant with the ``coding style''
used for start-stop-daemon.

  I would also love to have a start-stop-daemon with reload
and some lightweight monitoring features, either in the main
dpkg package, or as an enhanced-start-stop-daemon package.

Cheers, 
Carlo


-- System Information:
Debian Release: testing/unstable
  APT prefers unstable
  APT policy: (500, 'unstable'), (500, 'testing')
Architecture: i386 (i686)
Kernel: Linux 2.6.8
Locale: LANG=C, LC_CTYPE=C

Versions of packages dpkg depends on:
ii  dselect                     1.10.20      a user tool to manage Debian packa
ii  libc6                       2.3.2.ds1-17 GNU C Library: Shared libraries an

-- no debconf information

[dpkg-start-stop-daemon.02.diff (text/x-c, attachment)]

Message #12 received at 302079@bugs.debian.org (full text, mbox, reply):

From: Raphael Hertzog <hertzog@debian.org>

To: Carlo Contavalli <ccontavalli@debian.org>, 302079@bugs.debian.org

Subject: Re: Bug#302079: dpkg: [PATCH] setting ulimit from start-stop-daemon

Date: Fri, 4 Jul 2008 18:11:12 +0200

Hi Carlo,

sorry for the delay in getting back to you but dpkg has many open bugs...

On Wed, 30 Mar 2005, Carlo Contavalli wrote:
> I'm a DD too, and if there is any chance for it to be included
> in the ``upstream'' dpkg, I'd be willing to work on writing
> documentation, verify it works under kfreebsd and hurd, update
> it for your current development tree and generally verify its
> correct behavior.

I like the idea of this patch and it makes much sense to me. So if you're
still interested in updating it and getting it merged, feel free to jump
in! :)

dpkg is now managed with git, see
http://wiki.debian.org/Teams/Dpkg/GitUsage

>   I don't like the resulting code pretty much,

Why didn't you try to understand what you didn't like and fix it? :)

>   I would also love to have a start-stop-daemon with reload
> and some lightweight monitoring features, either in the main
> dpkg package, or as an enhanced-start-stop-daemon package.

One thing at a time... :) and it might be that start-stop-daemon will
be mainly useless if we switch to upstart one day (which has such
respawn/restart features).

Anyway, here are a few comments but take them with a grain of salt as I'm
not very used to the dpkg codebase yet.

> +AC_CHECK_HEADERS(sys/time.h sys/resource.h unistd.h)

Do we really need to check for those headers? AFAIK, they are mandated by
POSIX.

> +#if defined(HAVE_SYS_TIME_H) && defined(HAVE_SYS_RESOURCE_H) && \
> +    defined(HAVE_UNISTD_H) && defined(HAVE_SETRLIMIT) && defined(HAVE_GETRLIMIT)
> +
> +#  define SSD_SETRLIMIT 1
> +
> +#  include <sys/time.h>
> +#  include <sys/resource.h>
> +#endif

If you keep the header checks, just protect each include by its HAVE_*
variable. And use directly the checks HAVE_(S|G)ETRLIMIT in the code
instead of using the intermediary SSD_SETRLIMIT.

> +#ifdef SSD_SETRLIMIT
> +static int
> +do_getlimit(FILE * f, char ch, int * resource, const char ** rname) {

See below my remark about the parsing. This should be a simple
function to convert limit name ("as") into the corresponding limit
identifier (RLIMIT_AS) and nothing more complicated.

[ Skip the rest of this function ]

> +static void
> +do_loadlimits(void) 
> +{
> +  char * look=execname;

You should use "startas" here. It what's is really used
and should be always set with --start. If it's not set, you
shouldn't do anything anyway.

> +  if(!look) {
> +    if(!startas)
> +      return;
> +
> +    look=startas;
> +  }

No need for a fallback with startas, direct stop if not set.

> +    /* Calculate name of configuration file */
> +  name=strrchr(look, '/');
> +  if(name)
> +    name=name+1;
> +  else
> +    name=look;

A simple "name = name ? name+1 : look" would be nicer.

> +    /* Calculate name of the file to be read */
> +  path=xmalloc(sizeof(SSD_SETRLIMIT_PATH)+strlen(name)+1);
> +  sprintf(path, SSD_SETRLIMIT_PATH "/%s", name);

Wouldn't a fixed-size static buffer be simpler (with snprintf)?
FILENAME_MAX / PATH_MAX?

It's not like we'll ever reach any limit with length of daemon names.

> +    /* name of file/path to open */ 
> +  f=fopen(path, "r");
> +  if(!f) {
> +    if(errno == ENOENT) {
> +      if(quietmode < 0)
> +        printf("%s: not loading limits -- missing file '%s'\n", name, path);
> +      free(path);
> +      return;
> +    }
> +
> +    if(quietmode <= 0)
> +      printf("%s: couldn't open limits file -- %s\n", name, strerror(errno)); 
> +    free(path);
> +    return;
> +  }

Shouldn't we raise an error with fatal instead of silently continuing if
there's another problem (say a permission problem) ?

(I'm not sure of what's best)

> +
> +    /* Ok, file has been opened, read limits */
> +  while(1) {
> +
> +      /* Skip blanks */
> +    while((ch=fgetc(f)) != EOF && (ch == '\t' || ch == ' ' || ch == '\n'))  {
> +      if(ch == '\n')
> +	line++;
> +    }

Much of that parsing seems fragile/ugly to me. May I suggest to use
fgets() to read a line and then sscanf() to analyze it (with strcmp() to
verify if each field correspond to a valid value)?

And if the first field start with "#" then skip the line.
Same goes if sscanf has not been able to extract the 3 fields.

[ Skipping the rest of the function ] 

The parsing should happen first and then the change of limits. Both
shouldn't be too tightly mixed as they are now.

> +	    /* Verify SOFT limit is <= of the HARD limit */
> +	  *lvalue=(rlim_t)value;
> +	  if(lvalue == &limit.rlim_max && limit.rlim_cur > limit.rlim_max) {
> +	    if(quietmode < 0)
> +	      printf("%s: SOFT LIMIT was > than HARD LIMIT -- decreased to HARD LIMIT value on %s:%d (%s)\n",
> +		   name, path, line, lname);
> +
> +	    limit.rlim_cur=limit.rlim_max;
> +	  }

If the hard/soft limits are not set together, you might have troubles
due to incompatibilites between the current values and the new values...
in particular if the soft/hard limit setting happens with two calls to
setrlimit, no?

So maybe it's best to parse the whole file before doing the setrlimit
calls.

Cheers,
-- 
Raphaël Hertzog

Le best-seller français mis à jour pour Debian Etch :
http://www.ouaza.com/livre/admin-debian/

Send a report that this bug log contains spam.