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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

Nacked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

> +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.

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

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

Thanks,
Ian.

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