From mboxrd@z Thu Jan  1 00:00:00 1970
From: Olaf Hering <olaf@aepfle.de>
Subject: Re: [PATCH 7/7] tools/hotplug: add wrapper to start
	xenstored
Date: Wed, 7 Jan 2015 10:49:38 +0100
Message-ID: <20150107094938.GD12049@aepfle.de>
References: <1418988333-5404-1-git-send-email-olaf@aepfle.de>
	<1418988333-5404-8-git-send-email-olaf@aepfle.de>
	<21675.63511.6868.847055@mariner.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Content-Disposition: inline
In-Reply-To: <21675.63511.6868.847055@mariner.uk.xensource.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>, Ian Campbell <ian.campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, xen-devel@lists.xen.org, m.a.young@durham.ac.uk
List-Id: xen-devel@lists.xenproject.org

On Tue, Jan 06, Ian Jackson wrote:

> Olaf Hering writes ("[PATCH 7/7] tools/hotplug: add wrapper to start xenstored"):
> > The shell wrapper in xenstored.service does not handle XENSTORE_TRACE.
> ...
> > +XENSTORED_LIBEXEC = xenstored.sh
> 
> Should be in /etc as previously discussed.  Previously I wrote:
> 
>   Bottom line: as relevant maintainer, I'm afraid I'm going to insist
>   that this script be in /etc.
> 
> I'm disappointed.  It is not acceptable to resubmit a change ignoring
> such unequivocal feedback.

Plain "/etc" wont work, I think. /etc/xen/scripts perhaps? But see my
other reply to IanC, maybe there is a way to avoid the wrapper.

And after having some time to think about this: If one has a need to
adjust something, then this could be done in the xencommons script right
away. In other words, the modification can be done there instead of
calling the wrapper.

> Nacked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> > +hotplug/Linux/xenstored.sh
> 
> Although many of the existing hotplug scripts have this notion of
> calling things "foo.sh" because they happen to be written in shell, I
> think this is bad practice.
> 
> I would prefer xenstored-wrap or some such.  (My co-maintainers may
> disagree...)  But this is a bit of a bikeshed issue.

I agree. Initally I had xenstored-launcher in mind.

> >  		    echo -n Starting $XENSTORED...
> > -		    $XENSTORED --pid-file /var/run/xenstored.pid $XENSTORED_ARGS
> > +		    XENSTORED=$XENSTORED \
> > +		    XENSTORED_TRACE=$XENSTORED_TRACE \
> > +		    XENSTORED_ARGS=$XENSTORED_ARGS \
> > +		    ${LIBEXEC_BIN}/xenstored.sh --pid-file /var/run/xenstored.pid
> 
> It might be easier to "." xenstore-wrap.  Failing that using `export'
> will avoid this rather odd and repetitive style.

I think thats a good idea. Something like this may work, doing the "."
and the "exec" in the subshell:

 (
 set -- --pid-file /var/run/xenstored.pid
 . xenstored.sh 
 )


> > diff --git a/tools/hotplug/Linux/xenstored.sh.in b/tools/hotplug/Linux/xenstored.sh.in
> > new file mode 100644
> > index 0000000..dc806ee
> > --- /dev/null
> > +++ b/tools/hotplug/Linux/xenstored.sh.in
> > @@ -0,0 +1,6 @@
> > +#!/bin/sh
> > +if test -n "$XENSTORED_TRACE"
> > +then
> > +	XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log"
> > +fi
> > +exec $XENSTORED $@ $XENSTORED_ARGS
> 
> This should probably have "" around "$@" just in case.

Ok. 


I will wait for results from SELinux testing before respinning this
patch.

Olaf