Re: [Xen-devel] [PATCH v4 13/15] systemd: add xen systemd service and module files

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Jacek Konieczny writes ("Re: [PATCH v4 13/15] systemd: add xen systemd service 
and module files"):
> On 05/12/14 16:11, Ian Jackson wrote:
> > Blimey.  All of this to replace one line in a shell script.
> > 
> > Can you explain why this needs to be broken out into a separate
> > systemd service ?
> 
> Because that is how mount points are configured in systemd.
> 
> One could drop /etc/fstab all together and use a shell script to mount
> every file system. Would that be better?
> 
> In fact mounting /proc/xen from within a script instead of using fstab
> entry is hack required because /etc/fstab cannot be reliably updated on
> Xen install or activation.

Arguably, yes.

> The systemd mount units seem verbose, but they contain not much more
> than an /etc/fstab line and allow more flexibility (conditions and
> ordering).

My question was whether the same ordering and conditionality could be
left in an existing script without causing bugs.  That would be
preferable IMO.

> > Could it be bundled into some script which is already being run ?
> 
> No. Scripts are bad way for configuring mounts. Doing it in the script
> is the necessary evil in systems without drop-in configuration like
> systemd provides.

I'm afraid that this isn't a reason, it's just an assertion.

> > This should use a non-forking startup protocol, rather than pid files.
> 
> I would say more: it seems weird to use both 'Type=simple' and
> 'PIDFile=' â PIDFile seems unnecessary here. sd_notify support
> (Type=notify) would be the best option here, but it must be implemented
> in the daemon.

I would not object to sd_notify support.

> > xenstore isn't a file system.  The /var/run area it uses is just
> > something it needs for its private data.
> > 
> >> diff --git a/tools/hotplug/Linux/systemd/xenstored.socket.in 
> >> b/tools/hotplug/Linux/systemd/xenstored.socket.in
> >> +[Socket]
> >> +ListenStream=/var/run/xenstored/socket
> >> +ListenStream=/var/run/xenstored/socket_ro
> > 
> > AIUI the systemd socket activation protocol requires the systemd
> > config and the daemon code/configuration to correspond.
> 
> The daemon may have no own configuration when the systemd-provided
> sockets are used â the daemon does not need to know the paths, just that
> the first socked is read-write, and the other is read-only.

Indeed.  But that information is spread in multiple places and I think
it should be written down clearly.

And where that's helpful these multiple places should refer to each
other.  Finding the systemd config given the code will be
straightforward.  The other way around is less clear - particularly
given that there are two xenstored implementations and a change to
this startup arrangement might involve changing both.

> > There should be a comment explaining where the corresponding
> > configuration in xenstored is (ie referring to the code in both
> > daemons listing the sockets).
> 
> Comment about the expected sockets would not hurt, I guess.

Right, and a reference to the places in the code where it's
implemented.

Thanks,
Ian.

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