Re: [Xen-devel] [PATCH] xenconsoled: Remove unexpected daemonize behavior

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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

> -- 
> Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel