[PATCH 0/2] Introductions, some tweaks to ulogd

[PATCH 0/2] Introductions, some tweaks to ulogd

[Chris Boot]

Hi folks,

I am working on packaging ulogd-2.x for Debian, and currently have an initial
set of packages uploaded and waiting for ftpmaster review in the NEW queue.
I'd like to say hello to all folks involved with ulogd, and thanks for all the
hard work getting it to where it is. Hopefully we can work together and make
ulogd-2.x a first class citizen in Debian.

I intend to be a fully cooperative downstream maintainer, and to prove my
intentions here are a couple of patches that I've been working on for the next
upload of ulogd2.

The first patch simply fixes the deamon being able to nice() itself when run
with the --uid argument. Currently the code runs nice() after setuid(); this
fails to increase its priority as it is no longer root. The patch simply moves
the code around a little to fix this.

The second patch adds a --pidfile option to ulogd, which enables it to write a
PID file to somewhere like /var/run (or just /run). This is very useful within
init scripts to make sure you're sending signals to the correct process.

Cheers,
Chris

Chris Boot (2):
  ulogd: Perform nice() before giving up root
  ulogd: Implement PID file writing

 src/ulogd.c |   89 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 81 insertions(+), 8 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] ulogd: Perform nice() before giving up root

[Chris Boot]

The daemon code currently tries to nice(-1) just after having given up root
privileges, which fails. This patch moves the nice(-1) call to just before
the code that gives up the required privileges.

Signed-off-by: Chris Boot <bootc@bootc.net>
---
 src/ulogd.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/ulogd.c b/src/ulogd.c
index b28d0f8..8a144e3 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -1235,6 +1235,13 @@ int main(int argc, char* argv[])
 		warn_and_exit(daemonize);
 	}
 
+	errno = 0;
+	if (nice(-1) == -1) {
+		if (errno != 0)
+			ulogd_log(ULOGD_ERROR, "Could not nice process: %s\n",
+				  strerror(errno));
+	}
+
 	if (change_uid) {
 		ulogd_log(ULOGD_NOTICE, "Changing UID / GID\n");
 		if (setgid(gid)) {
@@ -1261,13 +1268,6 @@ int main(int argc, char* argv[])
 		}
 	}
 
-	errno = 0;
-	if (nice(-1) == -1) {
-		if (errno != 0)
-			ulogd_log(ULOGD_ERROR, "Could not nice process: %s\n",
-				  strerror(errno));
-	}
-
 
 	if (daemonize){
 		if (fork()) {
-- 
1.7.10.4

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

[Chris Boot]

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;
+	}
+
+	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);
-- 
1.7.10.4

Re: [PATCH 2/2] ulogd: Implement PID file writing

[Pablo Neira Ayuso]

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.

Re: [PATCH 2/2] ulogd: Implement PID file writing

[Chris Boot]

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

-- 
Chris Boot
bootc@bootc.net

Re: [PATCH 2/2] ulogd: Implement PID file writing

[Pablo Neira Ayuso]

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.

Re: [PATCH 2/2] ulogd: Implement PID file writing

[Chris Boot]

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

-- 
Chris Boot
bootc@bootc.net

Re: [PATCH 2/2] ulogd: Implement PID file writing

[Pablo Neira Ayuso]

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.

Re: [PATCH 2/2] ulogd: Implement PID file writing

[Chris Boot]

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

-- 
Chris Boot
bootc@bootc.net

Re: [PATCH 2/2] ulogd: Implement PID file writing

[Eric Leblond]

[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH 2/2] ulogd: Implement PID file writing

[Eric Leblond]

[-- Attachment #1: Type: text/plain, Size: 5822 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH 2/2] ulogd: Implement PID file writing

[Chris Boot]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

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

-- 
Chris Boot
bootc@bootc.net


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

Re: [PATCH 2/2] ulogd: Implement PID file writing

[Pablo Neira Ayuso]

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

Re: [PATCH 2/2] ulogd: Implement PID file writing

[Pablo Neira Ayuso]

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.

Re: [PATCH 2/2] ulogd: Implement PID file writing

[Chris Boot]

[-- Attachment #1: Type: text/plain, Size: 6833 bytes --]

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

-- 
Chris Boot
bootc@bootc.net


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

[PATCH v2] ulogd: Implement PID file writing

[Chris Boot]

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

Changes since v1:
 - Added documentation about the option to ulogd.8.
 - Move check for NULL ulogd_pidfile into main(), so it's more obvious that the
   code does nothing unless the --pidfile option is present.
 - Check for stale PID files and overwrite if that's the case, instead of
   bailing out.
 - Lock the pidfile with fcntl(); this is a good check to guard against running
   multiple instances with the same pidfile. (from atd)
 - Only unlink the pidfile at exit if we have created it. This prevents us
   removing another running ulogd process's pidfile if we exit because there is
   another running instance.

 src/ulogd.c |  147 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 ulogd.8     |    3 ++
 2 files changed, 149 insertions(+), 1 deletion(-)

diff --git a/src/ulogd.c b/src/ulogd.c
index 8a144e3..b835220 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,11 +81,13 @@
 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;
 
 static int verbose = 0;
+static int created_pidfile = 0;
 
 /* linked list for all registered plugins */
 static LLIST_HEAD(ulogd_plugins);
@@ -94,6 +99,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 +463,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 +1010,131 @@ static int parse_conffile(const char *section, struct config_keyset *ce)
 	return 1;
 }
 
+/*
+ * Apply F_WRLCK to fd using fcntl().
+ *
+ * This function is copied verbatim from atd's daemon.c file, published under
+ * the GPL2+ license with the following copyright statement:
+ * Copyright (C) 1996 Thomas Koenig
+ */
+static int lock_fd(int fd)
+{
+	struct flock lock;
+
+	lock.l_type = F_WRLCK;
+	lock.l_whence = SEEK_SET;
+	lock.l_start = 0;
+	lock.l_len = 0;
+
+	return fcntl(fd, F_SETLK, &lock);
+}
+
+/*
+ * Manage ulogd's pidfile.
+ *
+ * This function is based on atd's daemon.c:daemon_setup() function, published
+ * under the GPL2+ license with the following copyright statement:
+ * Copyright (C) 1996 Thomas Koenig
+ */
+static int write_pidfile()
+{
+	int fd;
+	FILE *fp;
+	pid_t pid = -1;
+
+	fd = open(ulogd_pidfile, O_RDWR | O_CREAT | O_EXCL, 0644);
+	if (fd < 0) {
+		if (errno != EEXIST) {
+			ulogd_log(ULOGD_ERROR, "cannot open %s: %d\n",
+					ulogd_pidfile, errno);
+			return -1;
+		}
+
+		fd = open(ulogd_pidfile, O_RDWR);
+		if (fd < 0) {
+			ulogd_log(ULOGD_ERROR, "cannot open %s: %d\n",
+					ulogd_pidfile, errno);
+			return -1;
+		}
+
+		fp = fdopen(fd, "rw");
+		if (fp == NULL) {
+			ulogd_log(ULOGD_ERROR, "cannot fdopen %s: %d\n",
+					ulogd_pidfile, errno);
+			return -1;
+		}
+
+		if ((fscanf(fp, "%d", &pid) != 1) || (pid == getpid())
+				|| (lock_fd(fd) == 0)) {
+			ulogd_log(ULOGD_NOTICE,
+				"removing stale pidfile for pid %d\n", pid);
+
+			if (unlink(ulogd_pidfile) < 0) {
+				ulogd_log(ULOGD_ERROR, "cannot unlink %s: %d\n",
+						ulogd_pidfile, errno);
+				return -1;
+			}
+		} else {
+			ulogd_log(ULOGD_FATAL,
+				"another ulogd already running with pid %d\n",
+				pid);
+			return -1;
+		}
+
+		fclose(fp);
+		unlink(ulogd_pidfile);
+
+		fd = open(ulogd_pidfile, O_RDWR | O_CREAT | O_EXCL, 0644);
+
+		if (fd < 0) {
+			ulogd_log(ULOGD_ERROR,
+				"cannot open %s (2nd time round): %d\n",
+				ulogd_pidfile, errno);
+			return -1;
+		}
+	}
+
+	if (lock_fd(fd) < 0) {
+		ulogd_log(ULOGD_ERROR, "cannot lock %s: %d\n", ulogd_pidfile,
+				errno);
+		return -1;
+	}
+
+	fp = fdopen(fd, "w");
+	if (fp == NULL) {
+		ulogd_log(ULOGD_ERROR, "cannot fdopen %s: %d\n", ulogd_pidfile,
+				errno);
+		return -1;
+	}
+
+	fprintf(fp, "%d\n", getpid());
+	fflush(fp);
+
+	if (ftruncate(fileno(fp), ftell(fp)) < 0)
+		ulogd_log(ULOGD_NOTICE, "cannot ftruncate %s: %d\n",
+				ulogd_pidfile, errno);
+
+	/*
+	 * We do NOT close fd, since we want to keep the lock. However, we don't
+	 * want to keep the file descriptor in case of an exec().
+	 */
+	fcntl(fd, F_SETFD, FD_CLOEXEC);
+
+	created_pidfile = 1;
+
+	return 0;
+}
+
+static void cleanup_pidfile()
+{
+	if (!ulogd_pidfile || !created_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 +1213,8 @@ static void sigterm_handler(int signal)
 
 	config_stop();
 
+	cleanup_pidfile();
+
 	exit(0);
 }
 
@@ -1121,6 +1256,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 +1270,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 +1287,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 +1316,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 +1420,11 @@ int main(int argc, char* argv[])
 		setsid();
 	}
 
+	if (ulogd_pidfile) {
+		if (write_pidfile() < 0)
+			warn_and_exit(daemonize);
+	}
+
 	signal(SIGTERM, &sigterm_handler);
 	signal(SIGINT, &sigterm_handler);
 	signal(SIGHUP, &signal_handler);
diff --git a/ulogd.8 b/ulogd.8
index 9cbad7c..9d16aeb 100644
--- a/ulogd.8
+++ b/ulogd.8
@@ -57,6 +57,9 @@ change UID/GID
 .TP
 .B -i <pluginpath>, --info <pluginpath>
 display infos about plugin
+.TP
+.B -p <filename>, --pidfile <filename>
+record the ulogd process ID to the given file name
 .SH FILES
 .I /etc/ulogd.conf
 .br
-- 
1.7.10.4

Re: [PATCH 2/2] ulogd: Implement PID file writing

[Eric Leblond]

[-- Attachment #1: Type: text/plain, Size: 1166 bytes --]

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>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH v2] ulogd: Implement PID file writing

[Chris Boot]

On 12/05/13 13:47, 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.
>
> Signed-off-by: Chris Boot <bootc@bootc.net>
> ---
>
> Changes since v1:
>  - Added documentation about the option to ulogd.8.
>  - Move check for NULL ulogd_pidfile into main(), so it's more obvious that the
>    code does nothing unless the --pidfile option is present.
>  - Check for stale PID files and overwrite if that's the case, instead of
>    bailing out.
>  - Lock the pidfile with fcntl(); this is a good check to guard against running
>    multiple instances with the same pidfile. (from atd)
>  - Only unlink the pidfile at exit if we have created it. This prevents us
>    removing another running ulogd process's pidfile if we exit because there is
>    another running instance.
>
>  src/ulogd.c |  147 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  ulogd.8     |    3 ++
>  2 files changed, 149 insertions(+), 1 deletion(-)
>
> diff --git a/src/ulogd.c b/src/ulogd.c
> index 8a144e3..b835220 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,11 +81,13 @@
>  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;
>  
>  static int verbose = 0;
> +static int created_pidfile = 0;
>  
>  /* linked list for all registered plugins */
>  static LLIST_HEAD(ulogd_plugins);
> @@ -94,6 +99,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 +463,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 +1010,131 @@ static int parse_conffile(const char *section, struct config_keyset *ce)
>  	return 1;
>  }
>  
> +/*
> + * Apply F_WRLCK to fd using fcntl().
> + *
> + * This function is copied verbatim from atd's daemon.c file, published under
> + * the GPL2+ license with the following copyright statement:
> + * Copyright (C) 1996 Thomas Koenig
> + */
> +static int lock_fd(int fd)
> +{
> +	struct flock lock;
> +
> +	lock.l_type = F_WRLCK;
> +	lock.l_whence = SEEK_SET;
> +	lock.l_start = 0;
> +	lock.l_len = 0;
> +
> +	return fcntl(fd, F_SETLK, &lock);
> +}
> +
> +/*
> + * Manage ulogd's pidfile.
> + *
> + * This function is based on atd's daemon.c:daemon_setup() function, published
> + * under the GPL2+ license with the following copyright statement:
> + * Copyright (C) 1996 Thomas Koenig
> + */
> +static int write_pidfile()
> +{
> +	int fd;
> +	FILE *fp;
> +	pid_t pid = -1;
> +
> +	fd = open(ulogd_pidfile, O_RDWR | O_CREAT | O_EXCL, 0644);
> +	if (fd < 0) {
> +		if (errno != EEXIST) {
> +			ulogd_log(ULOGD_ERROR, "cannot open %s: %d\n",
> +					ulogd_pidfile, errno);
> +			return -1;
> +		}
> +
> +		fd = open(ulogd_pidfile, O_RDWR);
> +		if (fd < 0) {
> +			ulogd_log(ULOGD_ERROR, "cannot open %s: %d\n",
> +					ulogd_pidfile, errno);
> +			return -1;
> +		}
> +
> +		fp = fdopen(fd, "rw");
> +		if (fp == NULL) {
> +			ulogd_log(ULOGD_ERROR, "cannot fdopen %s: %d\n",
> +					ulogd_pidfile, errno);
> +			return -1;
> +		}
> +
> +		if ((fscanf(fp, "%d", &pid) != 1) || (pid == getpid())
> +				|| (lock_fd(fd) == 0)) {
> +			ulogd_log(ULOGD_NOTICE,
> +				"removing stale pidfile for pid %d\n", pid);
> +
> +			if (unlink(ulogd_pidfile) < 0) {
> +				ulogd_log(ULOGD_ERROR, "cannot unlink %s: %d\n",
> +						ulogd_pidfile, errno);
> +				return -1;
> +			}
> +		} else {
> +			ulogd_log(ULOGD_FATAL,
> +				"another ulogd already running with pid %d\n",
> +				pid);
> +			return -1;
> +		}
> +
> +		fclose(fp);
> +		unlink(ulogd_pidfile);
> +
> +		fd = open(ulogd_pidfile, O_RDWR | O_CREAT | O_EXCL, 0644);
> +
> +		if (fd < 0) {
> +			ulogd_log(ULOGD_ERROR,
> +				"cannot open %s (2nd time round): %d\n",
> +				ulogd_pidfile, errno);
> +			return -1;
> +		}
> +	}
> +
> +	if (lock_fd(fd) < 0) {
> +		ulogd_log(ULOGD_ERROR, "cannot lock %s: %d\n", ulogd_pidfile,
> +				errno);
> +		return -1;
> +	}
> +
> +	fp = fdopen(fd, "w");
> +	if (fp == NULL) {
> +		ulogd_log(ULOGD_ERROR, "cannot fdopen %s: %d\n", ulogd_pidfile,
> +				errno);
> +		return -1;
> +	}
> +
> +	fprintf(fp, "%d\n", getpid());
> +	fflush(fp);
> +
> +	if (ftruncate(fileno(fp), ftell(fp)) < 0)
> +		ulogd_log(ULOGD_NOTICE, "cannot ftruncate %s: %d\n",
> +				ulogd_pidfile, errno);
> +
> +	/*
> +	 * We do NOT close fd, since we want to keep the lock. However, we don't
> +	 * want to keep the file descriptor in case of an exec().
> +	 */
> +	fcntl(fd, F_SETFD, FD_CLOEXEC);
> +
> +	created_pidfile = 1;
> +
> +	return 0;
> +}
> +
> +static void cleanup_pidfile()
> +{
> +	if (!ulogd_pidfile || !created_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 +1213,8 @@ static void sigterm_handler(int signal)
>  
>  	config_stop();
>  
> +	cleanup_pidfile();
> +
>  	exit(0);
>  }
>  
> @@ -1121,6 +1256,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 +1270,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 +1287,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 +1316,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 +1420,11 @@ int main(int argc, char* argv[])
>  		setsid();
>  	}
>  
> +	if (ulogd_pidfile) {
> +		if (write_pidfile() < 0)
> +			warn_and_exit(daemonize);
> +	}
> +
>  	signal(SIGTERM, &sigterm_handler);
>  	signal(SIGINT, &sigterm_handler);
>  	signal(SIGHUP, &signal_handler);
> diff --git a/ulogd.8 b/ulogd.8
> index 9cbad7c..9d16aeb 100644
> --- a/ulogd.8
> +++ b/ulogd.8
> @@ -57,6 +57,9 @@ change UID/GID
>  .TP
>  .B -i <pluginpath>, --info <pluginpath>
>  display infos about plugin
> +.TP
> +.B -p <filename>, --pidfile <filename>
> +record the ulogd process ID to the given file name
>  .SH FILES
>  .I /etc/ulogd.conf
>  .br

Hi folks,

Any comments about my revised patch? Is this likely to be taken up into
ulogd?

Cheers,
Chris

-- 
Chris Boot
bootc@bootc.net

Re: [PATCH 1/2] ulogd: Perform nice() before giving up root

[Chris Boot]

On 11/05/13 18:01, Chris Boot wrote:
> The daemon code currently tries to nice(-1) just after having given up root
> privileges, which fails. This patch moves the nice(-1) call to just before
> the code that gives up the required privileges.
>
> Signed-off-by: Chris Boot <bootc@bootc.net>
> ---
>  src/ulogd.c |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/src/ulogd.c b/src/ulogd.c
> index b28d0f8..8a144e3 100644
> --- a/src/ulogd.c
> +++ b/src/ulogd.c
> @@ -1235,6 +1235,13 @@ int main(int argc, char* argv[])
>  		warn_and_exit(daemonize);
>  	}
>  
> +	errno = 0;
> +	if (nice(-1) == -1) {
> +		if (errno != 0)
> +			ulogd_log(ULOGD_ERROR, "Could not nice process: %s\n",
> +				  strerror(errno));
> +	}
> +
>  	if (change_uid) {
>  		ulogd_log(ULOGD_NOTICE, "Changing UID / GID\n");
>  		if (setgid(gid)) {
> @@ -1261,13 +1268,6 @@ int main(int argc, char* argv[])
>  		}
>  	}
>  
> -	errno = 0;
> -	if (nice(-1) == -1) {
> -		if (errno != 0)
> -			ulogd_log(ULOGD_ERROR, "Could not nice process: %s\n",
> -				  strerror(errno));
> -	}
> -
>  
>  	if (daemonize){
>  		if (fork()) {

Hi all,

I have had no comments about this patch since I submitted it; is it
likely to get pulled into ulogd?

Cheers,
Chris

-- 
Chris Boot
bootc@bootc.net

Re: [PATCH 1/2] ulogd: Perform nice() before giving up root

[Eric Leblond]

Hi,

On Fri, 2013-05-17 at 08:34 +0100, Chris Boot wrote:
> On 11/05/13 18:01, Chris Boot wrote:
> > The daemon code currently tries to nice(-1) just after having given up root
> > privileges, which fails. This patch moves the nice(-1) call to just before
> > the code that gives up the required privileges.
> >
> > Signed-off-by: Chris Boot <bootc@bootc.net>
> > ---
> >  src/ulogd.c |   14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/ulogd.c b/src/ulogd.c
> > index b28d0f8..8a144e3 100644
> > --- a/src/ulogd.c
> > +++ b/src/ulogd.c
> > @@ -1235,6 +1235,13 @@ int main(int argc, char* argv[])
> >  		warn_and_exit(daemonize);
> >  	}
> >  
> > +	errno = 0;
> > +	if (nice(-1) == -1) {
> > +		if (errno != 0)
> > +			ulogd_log(ULOGD_ERROR, "Could not nice process: %s\n",
> > +				  strerror(errno));
> > +	}
> > +
> >  	if (change_uid) {
> >  		ulogd_log(ULOGD_NOTICE, "Changing UID / GID\n");
> >  		if (setgid(gid)) {
> > @@ -1261,13 +1268,6 @@ int main(int argc, char* argv[])
> >  		}
> >  	}
> >  
> > -	errno = 0;
> > -	if (nice(-1) == -1) {
> > -		if (errno != 0)
> > -			ulogd_log(ULOGD_ERROR, "Could not nice process: %s\n",
> > -				  strerror(errno));
> > -	}
> > -
> >  
> >  	if (daemonize){
> >  		if (fork()) {
> 
> Hi all,
> 
> I have had no comments about this patch since I submitted it; is it
> likely to get pulled into ulogd?

Yes, it seems ok. I will give it some tests but it should be applied.

BR,

> 
> Cheers,
> Chris
> 

-- 
Eric Leblond <eric@regit.org>
Blog: https://home.regit.org/

Re: [PATCH v2] ulogd: Implement PID file writing

[Eric Leblond]

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

Hi,

Le vendredi 17 mai 2013 à 08:33 +0100, Chris Boot a écrit :
> On 12/05/13 13:47, 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.
> >
> > Signed-off-by: Chris Boot <bootc@bootc.net>

I've applied this patch to my tree and made incremental modification to
fix some issues as explained in the upcoming patch.

BR,
--
Eric

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[Ulogd PATCH] Improve pid file handling.

[Eric Leblond]

This patch improves latest patch by splitting in two part the pid
file creation. This allows to display a message to stdout when
ulogd can not be started. Another linked improvement is that the
plugin initialization is not done if the pid file existence will
result in a ulogd exit.

Signed-off-by: Eric Leblond <eric@regit.org>
---
 src/ulogd.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 54 insertions(+), 12 deletions(-)

diff --git a/src/ulogd.c b/src/ulogd.c
index 4309201..c1aba77 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -82,6 +82,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 int ulogd_pidfile_fd = -1;
 static FILE syslog_dummy;
 
 static int info_mode = 0;
@@ -1017,7 +1018,7 @@ static int parse_conffile(const char *section, struct config_keyset *ce)
  * the GPL2+ license with the following copyright statement:
  * Copyright (C) 1996 Thomas Koenig
  */
-static int lock_fd(int fd)
+static int lock_fd(int fd, int wait)
 {
 	struct flock lock;
 
@@ -1026,7 +1027,10 @@ static int lock_fd(int fd)
 	lock.l_start = 0;
 	lock.l_len = 0;
 
-	return fcntl(fd, F_SETLK, &lock);
+	if (wait)
+		return fcntl(fd, F_SETLKW, &lock);
+	else
+		return fcntl(fd, F_SETLK, &lock);
 }
 
 /*
@@ -1036,12 +1040,15 @@ static int lock_fd(int fd)
  * under the GPL2+ license with the following copyright statement:
  * Copyright (C) 1996 Thomas Koenig
  */
-static int write_pidfile()
+static int create_pidfile()
 {
 	int fd;
 	FILE *fp;
 	pid_t pid = -1;
 
+	if (!ulogd_pidfile)
+		return 0;
+
 	fd = open(ulogd_pidfile, O_RDWR | O_CREAT | O_EXCL, 0644);
 	if (fd < 0) {
 		if (errno != EEXIST) {
@@ -1061,13 +1068,14 @@ static int write_pidfile()
 		if (fp == NULL) {
 			ulogd_log(ULOGD_ERROR, "cannot fdopen %s: %d\n",
 					ulogd_pidfile, errno);
+			close(fd);
 			return -1;
 		}
 
 		if ((fscanf(fp, "%d", &pid) != 1) || (pid == getpid())
-				|| (lock_fd(fd) == 0)) {
+				|| (lock_fd(fd, 0) == 0)) {
 			ulogd_log(ULOGD_NOTICE,
-				"removing stale pidfile for pid %d\n", pid);
+				  "removing stale pidfile for pid %d\n", pid);
 
 			if (unlink(ulogd_pidfile) < 0) {
 				ulogd_log(ULOGD_ERROR, "cannot unlink %s: %d\n",
@@ -1078,9 +1086,12 @@ static int write_pidfile()
 			ulogd_log(ULOGD_FATAL,
 				"another ulogd already running with pid %d\n",
 				pid);
+			fclose(fp);
+			close(fd);
 			return -1;
 		}
 
+		close(fd);
 		fclose(fp);
 		unlink(ulogd_pidfile);
 
@@ -1094,16 +1105,42 @@ static int write_pidfile()
 		}
 	}
 
-	if (lock_fd(fd) < 0) {
-		ulogd_log(ULOGD_ERROR, "cannot lock %s: %d\n", ulogd_pidfile,
-				errno);
+	if (lock_fd(fd, 0) < 0) {
+		ulogd_log(ULOGD_ERROR, "cannot lock %s: %s\n", ulogd_pidfile,
+				strerror(errno));
+		close(fd);
+		return -1;
+	}
+	ulogd_pidfile_fd = fd;
+	return 0;
+}
+
+static int write_pidfile(int daemonize)
+{
+	FILE *fp;
+	if (!ulogd_pidfile)
+		return 0;
+
+	if (ulogd_pidfile_fd == -1) {
+		ulogd_log(ULOGD_ERROR, "unset pid file fd\n");
 		return -1;
 	}
 
-	fp = fdopen(fd, "w");
+	if (daemonize) {
+		/* relocking as lock is not inherited */
+		if (lock_fd(ulogd_pidfile_fd, 1) < 0) {
+			ulogd_log(ULOGD_ERROR, "cannot lock %s: %d\n", ulogd_pidfile,
+					errno);
+			close(ulogd_pidfile_fd);
+			return -1;
+		}
+	}
+
+	fp = fdopen(ulogd_pidfile_fd, "w");
 	if (fp == NULL) {
 		ulogd_log(ULOGD_ERROR, "cannot fdopen %s: %d\n", ulogd_pidfile,
 				errno);
+		close(ulogd_pidfile_fd);
 		return -1;
 	}
 
@@ -1118,7 +1155,7 @@ static int write_pidfile()
 	 * We do NOT close fd, since we want to keep the lock. However, we don't
 	 * want to keep the file descriptor in case of an exec().
 	 */
-	fcntl(fd, F_SETFD, FD_CLOEXEC);
+	fcntl(ulogd_pidfile_fd, F_SETFD, FD_CLOEXEC);
 
 	created_pidfile = 1;
 
@@ -1350,6 +1387,11 @@ int main(int argc, char* argv[])
 		loglevel_ce.u.value = loglevel;
 		loglevel_ce.flag |= CONFIG_FLAG_VAL_PROTECTED;
 
+	if (ulogd_pidfile) {
+		if (create_pidfile() < 0)
+			warn_and_exit(0);
+	}
+
 	if (daemonize && verbose) {
 		verbose = 0;
 		ulogd_log(ULOGD_ERROR,
@@ -1421,8 +1463,8 @@ int main(int argc, char* argv[])
 	}
 
 	if (ulogd_pidfile) {
-		if (write_pidfile() < 0)
-			warn_and_exit(daemonize);
+		if (write_pidfile(daemonize) < 0)
+			warn_and_exit(0);
 	}
 
 	signal(SIGTERM, &sigterm_handler);
-- 
1.7.10.4

Re: [Ulogd PATCH] Improve pid file handling.

[Chris Boot]

On 19/05/13 20:22, Eric Leblond wrote:
> This patch improves latest patch by splitting in two part the pid
> file creation. This allows to display a message to stdout when
> ulogd can not be started. Another linked improvement is that the
> plugin initialization is not done if the pid file existence will
> result in a ulogd exit.
> 
> Signed-off-by: Eric Leblond <eric@regit.org>
> ---
>  src/ulogd.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 54 insertions(+), 12 deletions(-)
> 
> diff --git a/src/ulogd.c b/src/ulogd.c
> index 4309201..c1aba77 100644
> --- a/src/ulogd.c
> +++ b/src/ulogd.c
> @@ -82,6 +82,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 int ulogd_pidfile_fd = -1;
>  static FILE syslog_dummy;
>  
>  static int info_mode = 0;
> @@ -1017,7 +1018,7 @@ static int parse_conffile(const char *section, struct config_keyset *ce)
>   * the GPL2+ license with the following copyright statement:
>   * Copyright (C) 1996 Thomas Koenig
>   */
> -static int lock_fd(int fd)
> +static int lock_fd(int fd, int wait)
>  {
>  	struct flock lock;
>  
> @@ -1026,7 +1027,10 @@ static int lock_fd(int fd)
>  	lock.l_start = 0;
>  	lock.l_len = 0;
>  
> -	return fcntl(fd, F_SETLK, &lock);
> +	if (wait)
> +		return fcntl(fd, F_SETLKW, &lock);
> +	else
> +		return fcntl(fd, F_SETLK, &lock);
>  }
>  
>  /*
> @@ -1036,12 +1040,15 @@ static int lock_fd(int fd)
>   * under the GPL2+ license with the following copyright statement:
>   * Copyright (C) 1996 Thomas Koenig
>   */
> -static int write_pidfile()
> +static int create_pidfile()
>  {
>  	int fd;
>  	FILE *fp;
>  	pid_t pid = -1;
>  
> +	if (!ulogd_pidfile)
> +		return 0;
> +
>  	fd = open(ulogd_pidfile, O_RDWR | O_CREAT | O_EXCL, 0644);
>  	if (fd < 0) {
>  		if (errno != EEXIST) {
> @@ -1061,13 +1068,14 @@ static int write_pidfile()
>  		if (fp == NULL) {
>  			ulogd_log(ULOGD_ERROR, "cannot fdopen %s: %d\n",
>  					ulogd_pidfile, errno);
> +			close(fd);
>  			return -1;
>  		}
>  
>  		if ((fscanf(fp, "%d", &pid) != 1) || (pid == getpid())
> -				|| (lock_fd(fd) == 0)) {
> +				|| (lock_fd(fd, 0) == 0)) {
>  			ulogd_log(ULOGD_NOTICE,
> -				"removing stale pidfile for pid %d\n", pid);
> +				  "removing stale pidfile for pid %d\n", pid);
>  
>  			if (unlink(ulogd_pidfile) < 0) {
>  				ulogd_log(ULOGD_ERROR, "cannot unlink %s: %d\n",
> @@ -1078,9 +1086,12 @@ static int write_pidfile()
>  			ulogd_log(ULOGD_FATAL,
>  				"another ulogd already running with pid %d\n",
>  				pid);
> +			fclose(fp);
> +			close(fd);
>  			return -1;
>  		}
>  
> +		close(fd);
>  		fclose(fp);
>  		unlink(ulogd_pidfile);
>  
> @@ -1094,16 +1105,42 @@ static int write_pidfile()
>  		}
>  	}
>  
> -	if (lock_fd(fd) < 0) {
> -		ulogd_log(ULOGD_ERROR, "cannot lock %s: %d\n", ulogd_pidfile,
> -				errno);
> +	if (lock_fd(fd, 0) < 0) {
> +		ulogd_log(ULOGD_ERROR, "cannot lock %s: %s\n", ulogd_pidfile,
> +				strerror(errno));
> +		close(fd);
> +		return -1;
> +	}
> +	ulogd_pidfile_fd = fd;
> +	return 0;
> +}
> +
> +static int write_pidfile(int daemonize)
> +{
> +	FILE *fp;
> +	if (!ulogd_pidfile)
> +		return 0;
> +
> +	if (ulogd_pidfile_fd == -1) {
> +		ulogd_log(ULOGD_ERROR, "unset pid file fd\n");
>  		return -1;
>  	}
>  
> -	fp = fdopen(fd, "w");
> +	if (daemonize) {
> +		/* relocking as lock is not inherited */
> +		if (lock_fd(ulogd_pidfile_fd, 1) < 0) {
> +			ulogd_log(ULOGD_ERROR, "cannot lock %s: %d\n", ulogd_pidfile,
> +					errno);
> +			close(ulogd_pidfile_fd);
> +			return -1;
> +		}
> +	}
> +
> +	fp = fdopen(ulogd_pidfile_fd, "w");
>  	if (fp == NULL) {
>  		ulogd_log(ULOGD_ERROR, "cannot fdopen %s: %d\n", ulogd_pidfile,
>  				errno);
> +		close(ulogd_pidfile_fd);
>  		return -1;
>  	}
>  
> @@ -1118,7 +1155,7 @@ static int write_pidfile()
>  	 * We do NOT close fd, since we want to keep the lock. However, we don't
>  	 * want to keep the file descriptor in case of an exec().
>  	 */
> -	fcntl(fd, F_SETFD, FD_CLOEXEC);
> +	fcntl(ulogd_pidfile_fd, F_SETFD, FD_CLOEXEC);
>  
>  	created_pidfile = 1;
>  
> @@ -1350,6 +1387,11 @@ int main(int argc, char* argv[])
>  		loglevel_ce.u.value = loglevel;
>  		loglevel_ce.flag |= CONFIG_FLAG_VAL_PROTECTED;
>  
> +	if (ulogd_pidfile) {
> +		if (create_pidfile() < 0)
> +			warn_and_exit(0);
> +	}
> +
>  	if (daemonize && verbose) {
>  		verbose = 0;
>  		ulogd_log(ULOGD_ERROR,
> @@ -1421,8 +1463,8 @@ int main(int argc, char* argv[])
>  	}
>  
>  	if (ulogd_pidfile) {
> -		if (write_pidfile() < 0)
> -			warn_and_exit(daemonize);
> +		if (write_pidfile(daemonize) < 0)
> +			warn_and_exit(0);
>  	}
>  
>  	signal(SIGTERM, &sigterm_handler);
> 

Hi Eric,

I have been testing ulogd with your patch on top for some time now, and
it looks good. Thanks for the comments, review and your follow-up patch
- they are much appreciated.

Best regards,
Chris

-- 
Chris Boot
bootc@bootc.net