| Message ID | 1368291713-40132-3-git-send-email-bootc@bootc.net |
|---|---|
| State | Superseded |
| Headers | show |
Hi Chris, On Sat, May 11, 2013 at 06:01:53PM +0100, Chris Boot wrote: > The deamon currently does not have the ability to write a PID file to track its > process ID. This is very useful to an init script and to ensure there is only > one running instance. This patch implements this functionality. This belongs to the scope of the script and it doesn't seem to be useful for the internal operation of ulogd2. You can generate that PID file with something like: ps -ef | grep ulogd$ | awk '{ printf $2 }' And someone may want to have more than one instance of ulogd2, that's perfectly possible. Actually that's a good idea if you need to log both NFLOG and NFCT at the same time and you're running ulogd2 in a multi-core system. That will help to avoid hitting Netlink overrun errors. Regards. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/05/2013 20:21, Pablo Neira Ayuso wrote: > Hi Chris, > > On Sat, May 11, 2013 at 06:01:53PM +0100, Chris Boot wrote: >> The deamon currently does not have the ability to write a PID file to track its >> process ID. This is very useful to an init script and to ensure there is only >> one running instance. This patch implements this functionality. > This belongs to the scope of the script and it doesn't seem to be > useful for the internal operation of ulogd2. > > You can generate that PID file with something like: > > ps -ef | grep ulogd$ | awk '{ printf $2 }' > > And someone may want to have more than one instance of ulogd2, that's > perfectly possible. Actually that's a good idea if you need to log > both NFLOG and NFCT at the same time and you're running ulogd2 in a > multi-core system. That will help to avoid hitting Netlink overrun > errors. Hi Pablo, I'd argue exactly the opposite point: that when you want multiple instances a PID file can help you work out which is which. My patch adds an option that takes a filename argument, so two instances can write to two different PID files; grepping ps won't easily tell you which instance is the correct one (without resorting to grepping for command-line arguments). Additionally, using a PID file is completely optional and there is no change in behaviour unless you pass the argument. Cheers, Chris
On Sat, May 11, 2013 at 09:27:31PM +0100, Chris Boot wrote: [...] > Hi Pablo, > > I'd argue exactly the opposite point: that when you want multiple > instances a PID file can help you work out which is which. That new option may break existing setups with multiple instances. > My patch adds an option that takes a filename argument, so two > instances can write to two different PID files; grepping ps won't > easily tell you which instance is the correct one (without resorting > to grepping for command-line arguments). You can use pidof. Many other debian init scripts use it to obtain the process PID. Regards. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/05/2013 01:48, Pablo Neira Ayuso wrote: > On Sat, May 11, 2013 at 09:27:31PM +0100, Chris Boot wrote: > [...] >> Hi Pablo, >> >> I'd argue exactly the opposite point: that when you want multiple >> instances a PID file can help you work out which is which. > That new option may break existing setups with multiple instances. My patch explicitly doesn't change the behaviour of existing configurations. If you don't pass '--pidfile /path/to/file.pid', no pid file is written and there is no change in how ulogd works. >> My patch adds an option that takes a filename argument, so two >> instances can write to two different PID files; grepping ps won't >> easily tell you which instance is the correct one (without resorting >> to grepping for command-line arguments). > You can use pidof. Many other debian init scripts use it to obtain the > process PID. /usr/sbin/ulogd -d -c /etc/ulog/instance1.conf pidof ulogd > /run/ulog/instance1.pid # => 1234 /usr/sbin/ulogd -d -c /etc/ulog/instance2.conf pidof ulogd > /run/ulog/instance2.pid # => 1234 2345 The second pidof will list the pids of both instances of ulogd on the system. Without looking at all of the other pid files for other instances, how does it know which one was the one it just started? Chris
Hi Chris, On Sun, May 12, 2013 at 09:11:51AM +0100, Chris Boot wrote: > On 12/05/2013 01:48, Pablo Neira Ayuso wrote: > > On Sat, May 11, 2013 at 09:27:31PM +0100, Chris Boot wrote: > > [...] > >> Hi Pablo, > >> > >> I'd argue exactly the opposite point: that when you want multiple > >> instances a PID file can help you work out which is which. > > That new option may break existing setups with multiple instances. > > My patch explicitly doesn't change the behaviour of existing > configurations. If you don't pass '--pidfile /path/to/file.pid', no pid > file is written and there is no change in how ulogd works. Existing setups having already two ulogd2 instances will break, as they won't be passing --pidfile, thus clashing on the same default pid file. One of the instances will not proceed. They will have to add --pidfile to their scripts to get things back working. > >> My patch adds an option that takes a filename argument, so two > >> instances can write to two different PID files; grepping ps won't > >> easily tell you which instance is the correct one (without resorting > >> to grepping for command-line arguments). > > > > You can use pidof. Many other debian init scripts use it to obtain the > > process PID. > > /usr/sbin/ulogd -d -c /etc/ulog/instance1.conf > pidof ulogd > /run/ulog/instance1.pid # => 1234 > /usr/sbin/ulogd -d -c /etc/ulog/instance2.conf > pidof ulogd > /run/ulog/instance2.pid # => 1234 2345 > > The second pidof will list the pids of both instances of ulogd on the > system. Without looking at all of the other pid files for other > instances, how does it know which one was the one it just started? You don't need to know. Assuming these possible actions: /etc/init.d/ulogd2 {start|stop|restart|reload|force-reload|status} if you want to support multiple instances in your script, the action should apply to all of them. But anyway, I suggest that that the standalone debian installation sticks to one single instance at the same time, that's just fine for most people. Regards. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/05/2013 10:34, Pablo Neira Ayuso wrote: > Hi Chris, > > On Sun, May 12, 2013 at 09:11:51AM +0100, Chris Boot wrote: >> On 12/05/2013 01:48, Pablo Neira Ayuso wrote: >>> On Sat, May 11, 2013 at 09:27:31PM +0100, Chris Boot wrote: >>> [...] >>>> Hi Pablo, >>>> >>>> I'd argue exactly the opposite point: that when you want multiple >>>> instances a PID file can help you work out which is which. >>> That new option may break existing setups with multiple instances. >> My patch explicitly doesn't change the behaviour of existing >> configurations. If you don't pass '--pidfile /path/to/file.pid', no pid >> file is written and there is no change in how ulogd works. > Existing setups having already two ulogd2 instances will break, as > they won't be passing --pidfile, thus clashing on the same default pid > file. One of the instances will not proceed. They will have to add > --pidfile to their scripts to get things back working. There is no default pidfile. No --pidfile option: no pidfile is created. No change in behaviour. You only get the new behaviour if you explicitly add --pidfile to the arguments. > But anyway, I suggest that that the standalone debian installation > sticks to one single instance at the same time, that's just fine for > most people. The init script is indeed sticking to just one instance as-shipped. Chris
Hi, Le dimanche 12 mai 2013 à 11:34 +0200, Pablo Neira Ayuso a écrit : > Hi Chris, > > On Sun, May 12, 2013 at 09:11:51AM +0100, Chris Boot wrote: > > On 12/05/2013 01:48, Pablo Neira Ayuso wrote: > > > On Sat, May 11, 2013 at 09:27:31PM +0100, Chris Boot wrote: > > > [...] > > >> Hi Pablo, > > >> > > >> I'd argue exactly the opposite point: that when you want multiple > > >> instances a PID file can help you work out which is which. > > > That new option may break existing setups with multiple instances. > > > > My patch explicitly doesn't change the behaviour of existing > > configurations. If you don't pass '--pidfile /path/to/file.pid', no pid > > file is written and there is no change in how ulogd works. > > Existing setups having already two ulogd2 instances will break, as > they won't be passing --pidfile, thus clashing on the same default pid > file. One of the instances will not proceed. They will have to add > --pidfile to their scripts to get things back working. If I read the patch correctly, the pidfile is not created if the option is not given: In main we have: + if (write_pidfile() < 0) + warn_and_exit(daemonize); and in the write_pidfile() function we have: +static int write_pidfile() +{ + int pid_fp; + char pidtext[16]; + int len; + + if (!ulogd_pidfile) + return 0; So, the default behavior is not changed. If the default behavior is not changed, I see no problem in adding the pidfile feature. And if it can help to get ulogd into distributions, I'm in favor of it ;) But, as pointed out by Pablo's reading of the code, testing if we need to write the file only inside of write_pidfile() is a bit confusing something like: if (ulogd_pidfile) write_pidfile(); // add error handling here would be better. BR, -- Eric
Hi, Some comments inline. Le samedi 11 mai 2013 à 18:01 +0100, Chris Boot a écrit : > The deamon currently does not have the ability to write a PID file to track its > process ID. This is very useful to an init script and to ensure there is only > one running instance. This patch implements this functionality. > > Signed-off-by: Chris Boot <bootc@bootc.net> > --- > src/ulogd.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 74 insertions(+), 1 deletion(-) > > diff --git a/src/ulogd.c b/src/ulogd.c > index 8a144e3..982663f 100644 > --- a/src/ulogd.c > +++ b/src/ulogd.c > @@ -4,6 +4,7 @@ > * > * (C) 2000-2005 by Harald Welte <laforge@gnumonks.org> > * (C) 2013 by Eric Leblond <eric@regit.org> > + * (C) 2013 Chris Boot <bootc@bootc.net> > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 > @@ -55,12 +56,14 @@ > #include <signal.h> > #include <dlfcn.h> > #include <sys/types.h> > +#include <fcntl.h> > #include <dirent.h> > #include <getopt.h> > #include <pwd.h> > #include <grp.h> > #include <syslog.h> > #include <sys/time.h> > +#include <sys/stat.h> > #include <ulogd/conffile.h> > #include <ulogd/ulogd.h> > #ifdef DEBUG > @@ -78,6 +81,7 @@ > static FILE *logfile = NULL; /* logfile pointer */ > static char *ulogd_logfile = NULL; > static const char *ulogd_configfile = ULOGD_CONFIGFILE; > +static const char *ulogd_pidfile = NULL; > static FILE syslog_dummy; > > static int info_mode = 0; > @@ -94,6 +98,7 @@ static LLIST_HEAD(ulogd_pi_stacks); > static int load_plugin(const char *file); > static int create_stack(const char *file); > static int logfile_open(const char *name); > +static void cleanup_pidfile(); > > static struct config_keyset ulogd_kset = { > .num_ces = 4, > @@ -457,6 +462,8 @@ void __ulogd_log(int level, char *file, int line, const char *format, ...) > > static void warn_and_exit(int daemonize) > { > + cleanup_pidfile(); > + > if (!daemonize) { > if (logfile && !verbose) { > fprintf(stderr, "Fatal error, check logfile \"%s\"" > @@ -1002,6 +1009,62 @@ static int parse_conffile(const char *section, struct config_keyset *ce) > return 1; > } > > +static int write_pidfile() > +{ > + struct stat pid_st; > + int pid_fp; > + char pidtext[16]; > + int len; > + > + if (!ulogd_pidfile) > + return 0; > + > + if (stat(ulogd_pidfile, &pid_st) == 0 || errno != ENOENT) { > + ulogd_log(ULOGD_FATAL, "PID file %s exists, not starting\n", > + ulogd_pidfile); > + return -1; > + } If the file existe, an interesting improvement would be to test if the ulogd is really running. The following code do something like that: if (fscanf(pf, "%d", &pidv) == 1 && kill(pidv, 0) == 0) printf("already running"); If it is not the case, we can remove continue to proceed as we just have a ghost pidfile. > + > + pid_fp = open(ulogd_pidfile, O_WRONLY | O_CREAT | O_EXCL, 0644); > + if (pid_fp < 0) { > + ulogd_log(ULOGD_FATAL, "PID file %s could not be opened: %d\n", > + ulogd_pidfile, errno); > + return -1; > + } > + if (ftruncate(pid_fp, 0) != 0) { > + close(pid_fp); > + unlink(ulogd_pidfile); > + ulogd_log(ULOGD_FATAL, "PID file %s could not be truncated: %d\n", > + ulogd_pidfile, errno); > + return -1; > + } > + > + len = snprintf(pidtext, sizeof(pidtext), "%ld\n", (long)getpid()); > + > + if (write(pid_fp, pidtext, len) != len) { > + close(pid_fp); > + unlink(ulogd_pidfile); > + ulogd_log(ULOGD_FATAL, "PID file %s could not be written: %d\n", > + ulogd_pidfile, errno); > + return -1; > + } > + > + /* deliberately leave PID file open */ Why are you doing this ? > + return 0; > +} > + > +static void cleanup_pidfile() > +{ > + if (!ulogd_pidfile) > + return; > + > + if (unlink(ulogd_pidfile) != 0) > + ulogd_log(ULOGD_ERROR, "PID file %s could not be deleted: %d\n", > + ulogd_pidfile, errno); > +} > + > static void deliver_signal_pluginstances(int signal) > { > struct ulogd_pluginstance_stack *stack; > @@ -1080,6 +1143,8 @@ static void sigterm_handler(int signal) > > config_stop(); > > + cleanup_pidfile(); > + > exit(0); > } > > @@ -1121,6 +1186,7 @@ static void print_usage(void) > printf("\t-v --verbose\tOutput info on standard output\n"); > printf("\t-l --loglevel\tSet log level\n"); > printf("\t-c --configfile\tUse alternative Configfile\n"); > + printf("\t-p --pidfile\tRecord ulogd PID in file\n"); > printf("\t-u --uid\tChange UID/GID\n"); > printf("\t-i --info\tDisplay infos about plugin\n"); > } > @@ -1134,6 +1200,7 @@ static struct option opts[] = { > { "info", 1, NULL, 'i' }, > { "verbose", 0, NULL, 'v' }, > { "loglevel", 1, NULL, 'l' }, > + { "pidfile", 1, NULL, 'p' }, > {NULL, 0, NULL, 0} > }; > > @@ -1150,7 +1217,7 @@ int main(int argc, char* argv[]) > > ulogd_logfile = strdup(ULOGD_LOGFILE_DEFAULT); > > - while ((argch = getopt_long(argc, argv, "c:dvl:h::Vu:i:", opts, NULL)) != -1) { > + while ((argch = getopt_long(argc, argv, "c:p:dvl:h::Vu:i:", opts, NULL)) != -1) { > switch (argch) { > default: > case '?': > @@ -1179,6 +1246,9 @@ int main(int argc, char* argv[]) > case 'c': > ulogd_configfile = optarg; > break; > + case 'p': > + ulogd_pidfile = optarg; > + break; > case 'u': > change_uid = 1; > user = strdup(optarg); > @@ -1280,6 +1350,9 @@ int main(int argc, char* argv[]) > setsid(); > } > > + if (write_pidfile() < 0) As said in a previous mail, test that ulogd_pidfile is non NULL before calling the function. BR, -- Eric
On 12/05/2013 10:47, Eric Leblond wrote: > Hi, > > Le dimanche 12 mai 2013 à 11:34 +0200, Pablo Neira Ayuso a écrit : >> Hi Chris, >> >> On Sun, May 12, 2013 at 09:11:51AM +0100, Chris Boot wrote: >>> On 12/05/2013 01:48, Pablo Neira Ayuso wrote: >>>> On Sat, May 11, 2013 at 09:27:31PM +0100, Chris Boot wrote: >>>> [...] >>>>> Hi Pablo, >>>>> >>>>> I'd argue exactly the opposite point: that when you want multiple >>>>> instances a PID file can help you work out which is which. >>>> That new option may break existing setups with multiple instances. >>> >>> My patch explicitly doesn't change the behaviour of existing >>> configurations. If you don't pass '--pidfile /path/to/file.pid', no pid >>> file is written and there is no change in how ulogd works. >> >> Existing setups having already two ulogd2 instances will break, as >> they won't be passing --pidfile, thus clashing on the same default pid >> file. One of the instances will not proceed. They will have to add >> --pidfile to their scripts to get things back working. > > If I read the patch correctly, the pidfile is not created if the option > is not given: [snip] > But, as pointed out by Pablo's reading of the code, testing if we need > to write the file only inside of write_pidfile() is a bit confusing > something like: > > if (ulogd_pidfile) write_pidfile(); // add error handling here > > would be better. Yes, that makes sense, I'll change that and resubmit the patch. Chris
Hi Eric, On Sun, May 12, 2013 at 11:47:42AM +0200, Eric Leblond wrote: > Hi, > > Le dimanche 12 mai 2013 à 11:34 +0200, Pablo Neira Ayuso a écrit : > > Hi Chris, > > > > On Sun, May 12, 2013 at 09:11:51AM +0100, Chris Boot wrote: > > > On 12/05/2013 01:48, Pablo Neira Ayuso wrote: > > > > On Sat, May 11, 2013 at 09:27:31PM +0100, Chris Boot wrote: > > > > [...] > > > >> Hi Pablo, > > > >> > > > >> I'd argue exactly the opposite point: that when you want multiple > > > >> instances a PID file can help you work out which is which. > > > > That new option may break existing setups with multiple instances. > > > > > > My patch explicitly doesn't change the behaviour of existing > > > configurations. If you don't pass '--pidfile /path/to/file.pid', no pid > > > file is written and there is no change in how ulogd works. > > > > Existing setups having already two ulogd2 instances will break, as > > they won't be passing --pidfile, thus clashing on the same default pid > > file. One of the instances will not proceed. They will have to add > > --pidfile to their scripts to get things back working. > > If I read the patch correctly, the pidfile is not created if the option > is not given: > > In main we have: > > + if (write_pidfile() < 0) > + warn_and_exit(daemonize); > > and in the write_pidfile() function we have: > > +static int write_pidfile() > +{ > + int pid_fp; > + char pidtext[16]; > + int len; > + > + if (!ulogd_pidfile) > + return 0; > > So, the default behavior is not changed. > > If the default behavior is not changed, I see no problem in adding the > pidfile feature. And if it can help to get ulogd into distributions, I'm > in favor of it ;) I checking my /etc/init.d/ directory in my debian installation and many other daemon use pidof. Sorry, I still don't get why we need this extra code for something that really belongs to the scope of ulogd's shell script. Regards. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, May 12, 2013 at 10:38:14AM +0100, Chris Boot wrote: > On 12/05/2013 10:34, Pablo Neira Ayuso wrote: > > Hi Chris, > > > > On Sun, May 12, 2013 at 09:11:51AM +0100, Chris Boot wrote: > >> On 12/05/2013 01:48, Pablo Neira Ayuso wrote: > >>> On Sat, May 11, 2013 at 09:27:31PM +0100, Chris Boot wrote: > >>> [...] > >>>> Hi Pablo, > >>>> > >>>> I'd argue exactly the opposite point: that when you want multiple > >>>> instances a PID file can help you work out which is which. > >>> That new option may break existing setups with multiple instances. > >> My patch explicitly doesn't change the behaviour of existing > >> configurations. If you don't pass '--pidfile /path/to/file.pid', no pid > >> file is written and there is no change in how ulogd works. > > Existing setups having already two ulogd2 instances will break, as > > they won't be passing --pidfile, thus clashing on the same default pid > > file. One of the instances will not proceed. They will have to add > > --pidfile to their scripts to get things back working. > > There is no default pidfile. No --pidfile option: no pidfile is created. > No change in behaviour. > > You only get the new behaviour if you explicitly add --pidfile to the > arguments. You're right, I overlooked that. > > But anyway, I suggest that that the standalone debian installation > > sticks to one single instance at the same time, that's just fine for > > most people. > > The init script is indeed sticking to just one instance as-shipped. Assuming that, you'll have one single instance and pidof would be just fine. But I leave final decision to Eric. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/05/2013 10:53, Eric Leblond wrote: > Hi, > > Some comments inline. > > Le samedi 11 mai 2013 à 18:01 +0100, Chris Boot a écrit : >> The deamon currently does not have the ability to write a PID file to track its >> process ID. This is very useful to an init script and to ensure there is only >> one running instance. This patch implements this functionality. >> >> Signed-off-by: Chris Boot <bootc@bootc.net> >> --- >> src/ulogd.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 74 insertions(+), 1 deletion(-) >> >> diff --git a/src/ulogd.c b/src/ulogd.c >> index 8a144e3..982663f 100644 >> --- a/src/ulogd.c >> +++ b/src/ulogd.c >> @@ -4,6 +4,7 @@ >> * >> * (C) 2000-2005 by Harald Welte <laforge@gnumonks.org> >> * (C) 2013 by Eric Leblond <eric@regit.org> >> + * (C) 2013 Chris Boot <bootc@bootc.net> >> * >> * This program is free software; you can redistribute it and/or modify >> * it under the terms of the GNU General Public License version 2 >> @@ -55,12 +56,14 @@ >> #include <signal.h> >> #include <dlfcn.h> >> #include <sys/types.h> >> +#include <fcntl.h> >> #include <dirent.h> >> #include <getopt.h> >> #include <pwd.h> >> #include <grp.h> >> #include <syslog.h> >> #include <sys/time.h> >> +#include <sys/stat.h> >> #include <ulogd/conffile.h> >> #include <ulogd/ulogd.h> >> #ifdef DEBUG >> @@ -78,6 +81,7 @@ >> static FILE *logfile = NULL; /* logfile pointer */ >> static char *ulogd_logfile = NULL; >> static const char *ulogd_configfile = ULOGD_CONFIGFILE; >> +static const char *ulogd_pidfile = NULL; >> static FILE syslog_dummy; >> >> static int info_mode = 0; >> @@ -94,6 +98,7 @@ static LLIST_HEAD(ulogd_pi_stacks); >> static int load_plugin(const char *file); >> static int create_stack(const char *file); >> static int logfile_open(const char *name); >> +static void cleanup_pidfile(); >> >> static struct config_keyset ulogd_kset = { >> .num_ces = 4, >> @@ -457,6 +462,8 @@ void __ulogd_log(int level, char *file, int line, const char *format, ...) >> >> static void warn_and_exit(int daemonize) >> { >> + cleanup_pidfile(); >> + >> if (!daemonize) { >> if (logfile && !verbose) { >> fprintf(stderr, "Fatal error, check logfile \"%s\"" >> @@ -1002,6 +1009,62 @@ static int parse_conffile(const char *section, struct config_keyset *ce) >> return 1; >> } >> >> +static int write_pidfile() >> +{ >> + struct stat pid_st; >> + int pid_fp; >> + char pidtext[16]; >> + int len; >> + >> + if (!ulogd_pidfile) >> + return 0; >> + >> + if (stat(ulogd_pidfile, &pid_st) == 0 || errno != ENOENT) { >> + ulogd_log(ULOGD_FATAL, "PID file %s exists, not starting\n", >> + ulogd_pidfile); >> + return -1; >> + } > > If the file existe, an interesting improvement would be to test if the > ulogd is really running. The following code do something like that: > > if (fscanf(pf, "%d", &pidv) == 1 && kill(pidv, 0) == 0) > printf("already running"); > > If it is not the case, we can remove continue to proceed as we just have > a ghost pidfile. I've done some research on how PID files are handled by various daemons (previously I admit I only did a quick Googling), and it seems every implementation is different. It appears that what my current code does, which is to fail to start if a PID file exists at all, is not a common pattern in various daemons - so I'll change how that works. >> + >> + pid_fp = open(ulogd_pidfile, O_WRONLY | O_CREAT | O_EXCL, 0644); >> + if (pid_fp < 0) { >> + ulogd_log(ULOGD_FATAL, "PID file %s could not be opened: %d\n", >> + ulogd_pidfile, errno); >> + return -1; >> + } >> + if (ftruncate(pid_fp, 0) != 0) { >> + close(pid_fp); >> + unlink(ulogd_pidfile); >> + ulogd_log(ULOGD_FATAL, "PID file %s could not be truncated: %d\n", >> + ulogd_pidfile, errno); >> + return -1; >> + } >> + >> + len = snprintf(pidtext, sizeof(pidtext), "%ld\n", (long)getpid()); >> + >> + if (write(pid_fp, pidtext, len) != len) { >> + close(pid_fp); >> + unlink(ulogd_pidfile); >> + ulogd_log(ULOGD_FATAL, "PID file %s could not be written: %d\n", >> + ulogd_pidfile, errno); >> + return -1; >> + } >> + >> + /* deliberately leave PID file open */ > > Why are you doing this ? This seems to be a fairly common thing to do with pidfiles. I know atd and cron both do this, though looking at their code they also use fnctl/flock on the open filehandle to ensure exclusivity. I think I'll rewrite this code based on my research of these other daemons, and hopefully come up with something more useful and 'proper'. >> + return 0; >> +} >> + >> +static void cleanup_pidfile() >> +{ >> + if (!ulogd_pidfile) >> + return; >> + >> + if (unlink(ulogd_pidfile) != 0) >> + ulogd_log(ULOGD_ERROR, "PID file %s could not be deleted: %d\n", >> + ulogd_pidfile, errno); >> +} >> + >> static void deliver_signal_pluginstances(int signal) >> { >> struct ulogd_pluginstance_stack *stack; >> @@ -1080,6 +1143,8 @@ static void sigterm_handler(int signal) >> >> config_stop(); >> >> + cleanup_pidfile(); >> + >> exit(0); >> } >> >> @@ -1121,6 +1186,7 @@ static void print_usage(void) >> printf("\t-v --verbose\tOutput info on standard output\n"); >> printf("\t-l --loglevel\tSet log level\n"); >> printf("\t-c --configfile\tUse alternative Configfile\n"); >> + printf("\t-p --pidfile\tRecord ulogd PID in file\n"); >> printf("\t-u --uid\tChange UID/GID\n"); >> printf("\t-i --info\tDisplay infos about plugin\n"); >> } >> @@ -1134,6 +1200,7 @@ static struct option opts[] = { >> { "info", 1, NULL, 'i' }, >> { "verbose", 0, NULL, 'v' }, >> { "loglevel", 1, NULL, 'l' }, >> + { "pidfile", 1, NULL, 'p' }, >> {NULL, 0, NULL, 0} >> }; >> >> @@ -1150,7 +1217,7 @@ int main(int argc, char* argv[]) >> >> ulogd_logfile = strdup(ULOGD_LOGFILE_DEFAULT); >> >> - while ((argch = getopt_long(argc, argv, "c:dvl:h::Vu:i:", opts, NULL)) != -1) { >> + while ((argch = getopt_long(argc, argv, "c:p:dvl:h::Vu:i:", opts, NULL)) != -1) { >> switch (argch) { >> default: >> case '?': >> @@ -1179,6 +1246,9 @@ int main(int argc, char* argv[]) >> case 'c': >> ulogd_configfile = optarg; >> break; >> + case 'p': >> + ulogd_pidfile = optarg; >> + break; >> case 'u': >> change_uid = 1; >> user = strdup(optarg); >> @@ -1280,6 +1350,9 @@ int main(int argc, char* argv[]) >> setsid(); >> } >> >> + if (write_pidfile() < 0) > > As said in a previous mail, test that ulogd_pidfile is non NULL before > calling the function. Agreed. I'm changing this around. Chris
Hi, Le dimanche 12 mai 2013 à 12:50 +0200, Pablo Neira Ayuso a écrit : > On Sun, May 12, 2013 at 10:38:14AM +0100, Chris Boot wrote: > > On 12/05/2013 10:34, Pablo Neira Ayuso wrote: > > > Hi Chris, > > > > > > On Sun, May 12, 2013 at 09:11:51AM +0100, Chris Boot wrote: > > >> On 12/05/2013 01:48, Pablo Neira Ayuso wrote: > > >>> On Sat, May 11, 2013 at 09:27:31PM +0100, Chris Boot wrote: > > >>> [...] > > >>>> Hi Pablo, ... > You're right, I overlooked that. > > > > But anyway, I suggest that that the standalone debian installation > > > sticks to one single instance at the same time, that's just fine for > > > most people. > > > > The init script is indeed sticking to just one instance as-shipped. > > Assuming that, you'll have one single instance and pidof would be just > fine. But I leave final decision to Eric. This kind of code has already saved me some huge time in other projects I've worked on (mainly when debugging or on quick install). Furthermore, it is not painful to maintain once this is correctly done. When Chris patch is in a correct state, I will apply it. BR, -- Eric Leblond <eric@regit.org>
diff --git a/src/ulogd.c b/src/ulogd.c index 8a144e3..982663f 100644 --- a/src/ulogd.c +++ b/src/ulogd.c @@ -4,6 +4,7 @@ * * (C) 2000-2005 by Harald Welte <laforge@gnumonks.org> * (C) 2013 by Eric Leblond <eric@regit.org> + * (C) 2013 Chris Boot <bootc@bootc.net> * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 @@ -55,12 +56,14 @@ #include <signal.h> #include <dlfcn.h> #include <sys/types.h> +#include <fcntl.h> #include <dirent.h> #include <getopt.h> #include <pwd.h> #include <grp.h> #include <syslog.h> #include <sys/time.h> +#include <sys/stat.h> #include <ulogd/conffile.h> #include <ulogd/ulogd.h> #ifdef DEBUG @@ -78,6 +81,7 @@ static FILE *logfile = NULL; /* logfile pointer */ static char *ulogd_logfile = NULL; static const char *ulogd_configfile = ULOGD_CONFIGFILE; +static const char *ulogd_pidfile = NULL; static FILE syslog_dummy; static int info_mode = 0; @@ -94,6 +98,7 @@ static LLIST_HEAD(ulogd_pi_stacks); static int load_plugin(const char *file); static int create_stack(const char *file); static int logfile_open(const char *name); +static void cleanup_pidfile(); static struct config_keyset ulogd_kset = { .num_ces = 4, @@ -457,6 +462,8 @@ void __ulogd_log(int level, char *file, int line, const char *format, ...) static void warn_and_exit(int daemonize) { + cleanup_pidfile(); + if (!daemonize) { if (logfile && !verbose) { fprintf(stderr, "Fatal error, check logfile \"%s\"" @@ -1002,6 +1009,62 @@ static int parse_conffile(const char *section, struct config_keyset *ce) return 1; } +static int write_pidfile() +{ + struct stat pid_st; + int pid_fp; + char pidtext[16]; + int len; + + if (!ulogd_pidfile) + return 0; + + if (stat(ulogd_pidfile, &pid_st) == 0 || errno != ENOENT) { + ulogd_log(ULOGD_FATAL, "PID file %s exists, not starting\n", + ulogd_pidfile); + return -1; + } + + pid_fp = open(ulogd_pidfile, O_WRONLY | O_CREAT | O_EXCL, 0644); + if (pid_fp < 0) { + ulogd_log(ULOGD_FATAL, "PID file %s could not be opened: %d\n", + ulogd_pidfile, errno); + return -1; + } + + if (ftruncate(pid_fp, 0) != 0) { + close(pid_fp); + unlink(ulogd_pidfile); + ulogd_log(ULOGD_FATAL, "PID file %s could not be truncated: %d\n", + ulogd_pidfile, errno); + return -1; + } + + len = snprintf(pidtext, sizeof(pidtext), "%ld\n", (long)getpid()); + + if (write(pid_fp, pidtext, len) != len) { + close(pid_fp); + unlink(ulogd_pidfile); + ulogd_log(ULOGD_FATAL, "PID file %s could not be written: %d\n", + ulogd_pidfile, errno); + return -1; + } + + /* deliberately leave PID file open */ + + return 0; +} + +static void cleanup_pidfile() +{ + if (!ulogd_pidfile) + return; + + if (unlink(ulogd_pidfile) != 0) + ulogd_log(ULOGD_ERROR, "PID file %s could not be deleted: %d\n", + ulogd_pidfile, errno); +} + static void deliver_signal_pluginstances(int signal) { struct ulogd_pluginstance_stack *stack; @@ -1080,6 +1143,8 @@ static void sigterm_handler(int signal) config_stop(); + cleanup_pidfile(); + exit(0); } @@ -1121,6 +1186,7 @@ static void print_usage(void) printf("\t-v --verbose\tOutput info on standard output\n"); printf("\t-l --loglevel\tSet log level\n"); printf("\t-c --configfile\tUse alternative Configfile\n"); + printf("\t-p --pidfile\tRecord ulogd PID in file\n"); printf("\t-u --uid\tChange UID/GID\n"); printf("\t-i --info\tDisplay infos about plugin\n"); } @@ -1134,6 +1200,7 @@ static struct option opts[] = { { "info", 1, NULL, 'i' }, { "verbose", 0, NULL, 'v' }, { "loglevel", 1, NULL, 'l' }, + { "pidfile", 1, NULL, 'p' }, {NULL, 0, NULL, 0} }; @@ -1150,7 +1217,7 @@ int main(int argc, char* argv[]) ulogd_logfile = strdup(ULOGD_LOGFILE_DEFAULT); - while ((argch = getopt_long(argc, argv, "c:dvl:h::Vu:i:", opts, NULL)) != -1) { + while ((argch = getopt_long(argc, argv, "c:p:dvl:h::Vu:i:", opts, NULL)) != -1) { switch (argch) { default: case '?': @@ -1179,6 +1246,9 @@ int main(int argc, char* argv[]) case 'c': ulogd_configfile = optarg; break; + case 'p': + ulogd_pidfile = optarg; + break; case 'u': change_uid = 1; user = strdup(optarg); @@ -1280,6 +1350,9 @@ int main(int argc, char* argv[]) setsid(); } + if (write_pidfile() < 0) + warn_and_exit(daemonize); + signal(SIGTERM, &sigterm_handler); signal(SIGINT, &sigterm_handler); signal(SIGHUP, &signal_handler);
The deamon currently does not have the ability to write a PID file to track its process ID. This is very useful to an init script and to ensure there is only one running instance. This patch implements this functionality. Signed-off-by: Chris Boot <bootc@bootc.net> --- src/ulogd.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-)