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

["Luis R. Rodriguez" <mcgrof@xxxxxxxxxxxxxxxx>]

On Mon, May 12, 2014 at 8:36 AM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:
> 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.

Systemd provides a guarantee of dependencies being met if defined on
the unit files, which is what has been done. I'd put a bit more trust
over the unit service dependency mapping over LSB scripts.

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

Don't think about weighing service files vs scripts -- think about the
design goals that systemd is trying achieve, only then will replacing
scripts make some sort of sense.

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

sd_notify() support is indeed reasonable but at this point I'd prefer
if we provide support for it separately as an atomic evolution, I
don't see it as a requirement.

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

The complexity on xen is due to its wide support for different
systems, as xen grows to support two init systems for one OS this will
obviously grow, and making it a requirement to share can and should be
part of the design goals. This however should be clearly accepted then
as an overhead in terms of design, so give me a bit more time and I'll
bake something up as part of the next series.

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

Sure, this is crucial.

  Luis

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