Re: [PATCH 7/7] tools/hotplug: add wrapper to start xenstored

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Jan 07, 2015 at 10:49:38AM +0100, Olaf Hering wrote:
> 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.

It did work for me (I did an 'Tested-by') in my email.

Please do keep in mind that today is the last for commits for Xen 4.5.
No pressure :-)