Re: [PATCH] xenconsoled: Remove unexpected daemonize behavior

[Ross Lagerwall <ross.lagerwall@citrix.com>]

On 11/02/2015 04:37 PM, Wei Liu wrote:
> On Mon, Nov 02, 2015 at 11:17:38AM +0000, Ross Lagerwall wrote:
>> Previously, xenconsoled's daemonize function would do nothing if its
>> parent process is init (as it is under systemd but not sysv init).
>> This is confusing. Instead, always daemonize when asked to, but use the
>> "interactive" switch when running from the systemd service.
>>
>> Because a pidfile is only written when daemonizing, drop the pidfile
>> parameters from the service file (systemd keeps track of the pids
>> anyway).
>>
>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>> ---
>>   tools/console/daemon/utils.c                       | 4 ----
>>   tools/hotplug/Linux/systemd/xenconsoled.service.in | 3 +--
>>   2 files changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/tools/console/daemon/utils.c b/tools/console/daemon/utils.c
>> index dbb3b12..644f6af 100644
>> --- a/tools/console/daemon/utils.c
>> +++ b/tools/console/daemon/utils.c
>> @@ -52,10 +52,6 @@ void daemonize(const char *pidfile)
>>   	int i;
>>   	char buf[100];
>>
>> -	if (getppid() == 1) {
>> -		return;
>> -	}
>> -
>
> Er, I never noticed this before. And code archeology doesn't tell me why
> it was written like this either.

The fact that the service type was set to simple rather than forking was 
a sign...

>
>>   	if ((pid = fork()) > 0) {
>>   		exit(0);
>>   	} else if (pid == -1) {
>> diff --git a/tools/hotplug/Linux/systemd/xenconsoled.service.in b/tools/hotplug/Linux/systemd/xenconsoled.service.in
>> index cd282bf..8e333b1 100644
>> --- a/tools/hotplug/Linux/systemd/xenconsoled.service.in
>> +++ b/tools/hotplug/Linux/systemd/xenconsoled.service.in
>> @@ -10,10 +10,9 @@ Environment=XENCONSOLED_ARGS=
>>   Environment=XENCONSOLED_TRACE=none
>>   Environment=XENCONSOLED_LOG_DIR=@XEN_LOG_DIR@/console
>>   EnvironmentFile=@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
>> -PIDFile=@XEN_RUN_DIR@/xenconsoled.pid
>>   ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
>>   ExecStartPre=/bin/mkdir -p ${XENCONSOLED_LOG_DIR}
>> -ExecStart=@sbindir@/xenconsoled --pid-file @XEN_RUN_DIR@/xenconsoled.pid --log=${XENCONSOLED_TRACE} --log-dir=${XENCONSOLED_LOG_DIR} $XENCONSOLED_ARGS
>> +ExecStart=@sbindir@/xenconsoled -i --log=${XENCONSOLED_TRACE} --log-dir=${XENCONSOLED_LOG_DIR} $XENCONSOLED_ARGS
>>
>
> To the best of my knowledge this seems to conform with man 7 daemon in
> Linux, "New-Style Daemons" session:
>
> "For developing a new-style daemon, none of the initialization steps
> recommended for SysV daemons need to be implemented. New-style init
> systems such as systemd make all of them redundant. Moreover, since some
> of these steps interfere with process monitoring, file descriptor
> passing and other functionality of the init system, it is recommended
> not to execute them when run as new-style service."
>
> So the use of "-i" seems justified.
>

If a service needs to listen on a port or something and other services 
need to depend on it, then the preferred method would be using something 
like sd_notify. A less satisfactory approach would be to use forking, 
and then only writing the pidfile after the port is opened.

In this case, I don't think xenconsoled has any of these requirements so 
using Type=simple and keeping it in the foreground is the correct thing 
to do.

-- 
Ross Lagerwall