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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

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.
--
Ross Lagerwall

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