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.jack...@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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
