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

[Olaf Hering <olaf@aepfle.de>]

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