From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ross Lagerwall Subject: Re: [PATCH] xenconsoled: Remove unexpected daemonize behavior Date: Mon, 2 Nov 2015 16:45:37 +0000 Message-ID: <56379331.5050409@citrix.com> References: <1446463058-11967-1-git-send-email-ross.lagerwall@citrix.com> <20151102163741.GF18337@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20151102163741.GF18337@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: Stefano Stabellini , Ian Jackson , Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 >> --- >> 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