* [PATCH] xenconsoled: Remove unexpected daemonize behavior
@ 2015-11-02 11:17 Ross Lagerwall
2015-11-02 16:37 ` Wei Liu
2015-11-02 16:53 ` Ian Jackson
0 siblings, 2 replies; 6+ messages in thread
From: Ross Lagerwall @ 2015-11-02 11:17 UTC (permalink / raw)
To: xen-devel
Cc: Ian Jackson, Ross Lagerwall, Wei Liu, Ian Campbell, Stefano Stabellini
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;
- }
-
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
[Install]
WantedBy=multi-user.target
--
2.4.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xenconsoled: Remove unexpected daemonize behavior
2015-11-02 11:17 [PATCH] xenconsoled: Remove unexpected daemonize behavior Ross Lagerwall
@ 2015-11-02 16:37 ` Wei Liu
2015-11-02 16:45 ` Ross Lagerwall
2015-11-02 16:53 ` Ian Jackson
1 sibling, 1 reply; 6+ messages in thread
From: Wei Liu @ 2015-11-02 16:37 UTC (permalink / raw)
To: Ross Lagerwall
Cc: Ian Jackson, Stefano Stabellini, Wei Liu, Ian Campbell, xen-devel
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.
> 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.
I will wait for some input from Ian and Ian.
Wei.
> [Install]
> WantedBy=multi-user.target
> --
> 2.4.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xenconsoled: Remove unexpected daemonize behavior
2015-11-02 16:37 ` Wei Liu
@ 2015-11-02 16:45 ` Ross Lagerwall
2015-11-03 17:20 ` Wei Liu
0 siblings, 1 reply; 6+ messages in thread
From: Ross Lagerwall @ 2015-11-02 16:45 UTC (permalink / raw)
To: Wei Liu; +Cc: Stefano Stabellini, Ian Jackson, Ian Campbell, xen-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xenconsoled: Remove unexpected daemonize behavior
2015-11-02 11:17 [PATCH] xenconsoled: Remove unexpected daemonize behavior Ross Lagerwall
2015-11-02 16:37 ` Wei Liu
@ 2015-11-02 16:53 ` Ian Jackson
1 sibling, 0 replies; 6+ messages in thread
From: Ian Jackson @ 2015-11-02 16:53 UTC (permalink / raw)
To: Ross Lagerwall; +Cc: Stefano Stabellini, Wei Liu, Ian Campbell, xen-devel
Ross Lagerwall writes ("[PATCH] xenconsoled: Remove unexpected daemonize behavior"):
> 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.
It's quite bogglesome, indeed.
> 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).
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Thanks,
Ian.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xenconsoled: Remove unexpected daemonize behavior
2015-11-02 16:45 ` Ross Lagerwall
@ 2015-11-03 17:20 ` Wei Liu
2015-11-04 15:29 ` Ian Campbell
0 siblings, 1 reply; 6+ messages in thread
From: Wei Liu @ 2015-11-03 17:20 UTC (permalink / raw)
To: Ross Lagerwall
Cc: Ian Jackson, Stefano Stabellini, Wei Liu, Ian Campbell, xen-devel
On Mon, Nov 02, 2015 at 04:45:37PM +0000, Ross Lagerwall wrote:
> 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.
>
I think I'm convinced.
Acked-by: Wei Liu <wei.liu2@citrix.com>
> --
> Ross Lagerwall
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xenconsoled: Remove unexpected daemonize behavior
2015-11-03 17:20 ` Wei Liu
@ 2015-11-04 15:29 ` Ian Campbell
0 siblings, 0 replies; 6+ messages in thread
From: Ian Campbell @ 2015-11-04 15:29 UTC (permalink / raw)
To: Wei Liu, Ross Lagerwall; +Cc: Stefano Stabellini, Ian Jackson, xen-devel
On Tue, 2015-11-03 at 17:20 +0000, Wei Liu wrote:
> Acked-by: Wei Liu <wei.liu2@citrix.com>
Applied w/ this + Ian J's ack.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-11-04 15:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-02 11:17 [PATCH] xenconsoled: Remove unexpected daemonize behavior Ross Lagerwall
2015-11-02 16:37 ` Wei Liu
2015-11-02 16:45 ` Ross Lagerwall
2015-11-03 17:20 ` Wei Liu
2015-11-04 15:29 ` Ian Campbell
2015-11-02 16:53 ` Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.