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/ -- To UNSUBSCRIBE, email to [EMAIL PROTECTED] with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
