[PATCH] CONFIG_PID_FILE_PATH: new configuration option for pidfile paths

Anthony G. Basile basile at opensource.dyc.edu
Mon Dec 10 19:24:29 UTC 2012


On 12/08/2012 03:54 PM, Tito wrote:
> Hi,
> just a few comments on the patch.
>> diff --git a/Config.in b/Config.in
>> index 17bdc89..322343e 100644
>> --- a/Config.in
>> +++ b/Config.in
>> @@ -310,7 +310,18 @@ config FEATURE_PIDFILE
>>   	default y
>>   	help
>>   	  This option makes some applets (e.g. crond, syslogd, inetd) write
>> -	  a pidfile in /var/run. Some applications rely on them.
>> +	  a pidfile at the configured PID_FILE_PATH.  It has no effect
>
> +	  on applets which require pidfiles to run.

Thanks for catching that.


>>   	crondlog(LVL8 "crond (busybox "BB_VER") started, log level %d", G.log_level);
>>   	rescan_crontab_dir();
>> -	write_pidfile("/var/run/crond.pid");
>
>      Is there no way for the sake of clarity to show that
>      if FEATURE_PIDFILE is not set write_pidfile and remove_pidfile are no-ops,
>      as the definition to ((void)0)  is buried in libbb.h,
>      something like
>
>      if (ENABLE_FEATURE_PIDFILE)
> 	write_pidfile(CONFIG_PID_FILE_PATH"/crond.pid");
>
>     or uglier
>
> #if ENABLE_FEATURE_PIDFILE
> 	write_pidfile(CONFIG_PID_FILE_PATH"/crond.pid");
> #endif
>
>      This should not increase size but makes the code easier to understand.
>

The busybox code is dedicated to having two defines which can enable 
pidfile writing, ENABLE_FEATURE_PIDFILE which is set by user 
configuration and WANT_PIDFILE which is defined in those applets which 
require pidfiles to run.  I agree it would make the code a little easier 
to read, but we'd have to rewrite that logic throughout.  I'm not sure 
the benefits really merit that since its pretty easy to read as is.


>> diff --git a/util-linux/acpid.c b/util-linux/acpid.c
>> index 5d27929..d6216f4 100644
>> --- a/util-linux/acpid.c
>> +++ b/util-linux/acpid.c
>> @@ -235,7 +235,7 @@ int acpid_main(int argc UNUSED_PARAM, char **argv)
>>   	const char *opt_action = "/etc/acpid.conf";
>>   	const char *opt_map = "/etc/acpi.map";
>>   #if ENABLE_FEATURE_PIDFILE
>> -	const char *opt_pidfile = "/var/run/acpid.pid";
>
> Why not?
>
> 	const char *opt_pidfile = CONFIG_PID_FILE_PATH"/acpid.pid";
>

Heh, because I wasn't thinking.  Yeah obvious in retrospect.

>
> Just my little contribution.
>

Thanks, I'll include these with my next submission


-- 
Anthony G. Basile, Ph. D.
Chair of Information Technology
D'Youville College
Buffalo, NY 14201
(716) 829-8197


More information about the busybox mailing list