Patchwork [2/2] ulogd: Implement PID file writing

login
register
mail settings
Submitter Chris Boot
Date May 11, 2013, 5:01 p.m.
Message ID <1368291713-40132-3-git-send-email-bootc@bootc.net>
Download mbox | patch
Permalink /patch/243145/
State Superseded
Headers show

Comments

Chris Boot - May 11, 2013, 5:01 p.m.
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(-)
Pablo Neira - May 11, 2013, 7:21 p.m.
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
Chris Boot - May 11, 2013, 8:27 p.m.
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
Pablo Neira - May 12, 2013, 12:48 a.m.
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
Chris Boot - May 12, 2013, 8:11 a.m.
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
Pablo Neira - May 12, 2013, 9:34 a.m.
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
Chris Boot - May 12, 2013, 9:38 a.m.
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
Eric Leblond - May 12, 2013, 9:47 a.m.
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
Eric Leblond - May 12, 2013, 9:53 a.m.
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
Chris Boot - May 12, 2013, 10:08 a.m.
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
Pablo Neira - May 12, 2013, 10:49 a.m.
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
Pablo Neira - May 12, 2013, 10:50 a.m.
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
Chris Boot - May 12, 2013, 10:59 a.m.
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
Eric Leblond - May 12, 2013, 7:34 p.m.
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>

Patch

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);