Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Olaf Hering writes ("Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in 
systemd"):
> On Fri, Dec 05, Ian Jackson wrote:
> > I think the only way to make this work properly is to factor the
> > necessary parts out of init.d/xencommons into a new script which can
> > be used by both xencommons and systemd.  I'm not sure such a patch
> > would be appropriate for 4.5 at this stage.
> 
> I came up with this, it appears to work in my testing. Will do more
> testing later today.

Thanks.  I think this is going in roughly the right direction.

But: I think the script is rather over-engineered, and that it ought
to be in /etc.

> +     $(INSTALL_PROG) $(XENSTORED_LIBEXEC) $(DESTDIR)$(LIBEXEC_BIN)

Sysadmins might want to edit the script to do something we haven't
thought of.  So it should be in /etc where they can do so.

> diff --git a/tools/hotplug/Linux/xenstored.sh.in 
> b/tools/hotplug/Linux/xenstored.sh.in
> new file mode 100644
> index 0000000..11caf25
> --- /dev/null
> +++ b/tools/hotplug/Linux/xenstored.sh.in
...
> +    case "$1" in
> +    --exec)
> +        do_exec="exec"
> +    ;;
> +    --opt)
> +        opts=(${opts[@]} "$2")
> +        shift
> +    ;;
> +    esac
> +    shift
> +done

I don't think this script wants to contain an option parser!

> +. @XEN_SCRIPT_DIR@/hotplugpath.sh
...
> +test -f $xencommons_config/xencommons && . $xencommons_config/xencommons
...
> +    # Wait for xenstored to actually come up, timing out after 30 seconds
> +    while [ $time -lt $timeout ] && ! `${BINDIR}/xenstore-read -s / 
> >/dev/null 2>&1` ; do

I would have expected this new script to contain only the
functionality which was previously both in (a) the systemd unit and
(b) the traditional init script, and which you are removing from both
those places.  So I think you should be able to say "no ultimate
functional change" in your commit message.

The systemd unit doesn't currently contain anything messing about with
xenstore-read to detect when xenstored is working.  Is that a bug that
was previously in the systemd unit, or is it a mistake in your patch
that this is added here ?

Thanks,
Ian.

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