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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxx>
>> * (C) 2013 by Eric Leblond <eric@xxxxxxxxx>
>> + * (C) 2013 Chris Boot <bootc@xxxxxxxxx>
>> *
>> * 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@xxxxxxxxx
Attachment:
signature.asc
Description: OpenPGP digital signature
