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 Tue, Dec 09, Ian Jackson wrote:
> > But: I think the script is rather over-engineered, and that it ought
> > to be in /etc.
> 
> Why should the wrapper be in /etc?!

Because it is the last chance the admin has to adjust how xenstored is
run, which they ought to be able to do.

> xendomains isnt in /etc either.

xendomains is rather different.

> > > + $(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.
> 
> So they should mail here if they find substantial functionality missing.

That is no good because it doesn't allow them to make the change
immediately on their own system, in a way that won't be overwritten by
their system's package manager.

> Until then they continue to not enable xencommons and run their own
> startup script.

That is not a practical suggestion.

Bottom line: as relevant maintainer, I'm afraid I'm going to insist
that this script be in /etc.


> > I don't think this script wants to contain an option parser!
> 
> How should it handle exec vs. no-exec? Just a single yes/no knob, so
> essentially sysv vs systemd?

I was imagining a "named parameter" as SuS calls them.  One or both
the sites which run this wrapper script would pass an environment
variable.  Something like this in the script:

  $xenstored_do_exec $XENSTORED "$@" $XENSTORED_TRACE_ARGS $XENSTORED_ARGS

where the systemd unit simply sets `xenstored_do_exec=exec'.


> > 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 ?
> 
> No idea how long it takes to have a functional xenstored after running
> it. Perhaps the forking has some overhead and it returns earlier than it
> can process requests. If thats the reason why the loop exists in the
> sysv runlevel script then that loop should be used only without --no-fork.

I think it is up to you as the person proposing a change to explain
what the situation is and why your change is justified.  It's not
really satisfactory for a maintainer or reviewer to ask questions of a
submitter and get simply `I'm not sure ... perhaps ...' without some
kind of acknowledgement that more information (and perhaps research)
is needed before we can establish how the code should look.

In this case that means I think you should find out whether the lack
of this xenstore-read polling loop is actually a bug in the existing
systemd unit.  If (as I suspect) this is not a bug, then your code
motion should not change this aspect of the operation under systemd.

Thanks,
Ian.

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