Re: [Xen-devel] [PATCH] systemd: add support initial systemd service files

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

On Wed, Mar 12, 2014 at 1:16 AM, Jacek Konieczny <jajcus@xxxxxxxxxx> wrote:
> On 03/12/14 01:03, Luis R. Rodriguez wrote:
>> From: "Luis R. Rodriguez" <mcgrof@xxxxxxxx>
>>
>> I've tested these on an OpenSUSE Tumbleweed source install.
>> They are based on the ones from OpenSUSE but slightly modified.
>
> These seems quite wrong to me. Why create systemd services, while they
> are only wrappers to the old-style LSB scripts? What do we gain?

Huh? LSB? Systemd is here to stay. I don't see what you are trying to
say with LSB stuff.

> Systemd provides great process control and monitoring, but it can only
> do that if it starts the process by itself â it knows the main PID then,
> knows if the service is running, etc.

Now that sounds like a reasonable constructive argument to NACK a
patch, can you provide pointers to one service file that does that
properly as you are suggesting?

> systemd can do a better job handling old init.d scripts through its
> compatibility functions than when it is told to run such a script
> through ExecStart/ExecStop.

Cool, like which ones?

>> diff --git a/tools/hotplug/Linux/xencommons.service 
>> b/tools/hotplug/Linux/xencommons.service
>> new file mode 100644
>> index 0000000..8042b24
>> --- /dev/null
>> +++ b/tools/hotplug/Linux/xencommons.service
>> @@ -0,0 +1,12 @@
>> +[Unit]
>> +Description=Xencommons - Script to start and stop xenstored and xenconsoled
>> +ConditionPathIsDirectory=/proc/xen
>> +
>> +[Service]xenstored
>> +Type=oneshot
>> +RemainAfterExit=true
>> +ExecStart=-/etc/init.d/xencommons start
>> +ExecStop=/etc/init.d/xencommons stop
>
>
> This is especially bad: two processes started through one service, no
> startup status verified.
>
> The right thing to do is to provide separate systemd unit for xenstored
> and xenconsoled. If 'xencommons.service' is needed, to have a compatible
> service name with the LSB scripts, then it should be a dummy service with:
>
> Wants=xenstored.service xenconsoled.service
>
> And both xenstored.service and xenconsoled.service should have:
>
> PartOf=xencommons.service

Seems like you can provide a much better set of suitable scripts, mind
just taking them and fixing them or would you like me to take your
advice steps by step?

  Luis

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