El 08/09/16 a las 13:13, Lukas Slebodnik escribió:
On (08/09/16 12:29), Victor Tapia wrote:
> El 01/09/16 a las 14:50, Lukas Slebodnik escribió:
>> On (26/08/16 13:16), Victor Tapia wrote:
>>> Hi,
>>>
>>> Sorry for the late response, I've been out for nearly two weeks. I'm
>>> attaching a new version of the patch to review.
>>>
>>>> Does upstart/init script need to be updated as well?
>>>> We have some generic in src/sysv/sssd.in
>>>
>>> Upstart does need to keep track of the pidfile creation. I can't see a
>>> service file, so I'll prepare one.
>>>
>>> Regarding sysv, due to its sequential nature, it's dangerous to include
>>> an infinite loop because it could stuck the normal boot of the machine.
>>> I included a timeout in the loop to avoid this, please let me know what
>>> you think about it.
>>>
>>>> BTW the patch causes some failures for me on rhel6.
>>>> According to logs, the file /var/run/sssd.pid
>>>> did not exist in some cases.
>>>>
>>>> Please also remove changes in src/sysv/systemd/sssd.service.in
>>>> in next version. For systemd only I would prefer
"Type=notify".
>>>
>>> Done, I've switched to Type=notify for systemd.
>>>
>> I ran tests and I do not understand why but some tests on rhel6
>> were stucked in weird state. I'm not sure whether it's caused by your
patch
>> or by wrong expectation in tests. I will also try to test with 1.13.
>>
>> But so far it seems to work with systemd.
>>
>> LS
>>
>
> Hi Lukas,
>
> here's the last iteration of the patch for 1.14.2, including the upstart
> service script.
>
I noticed some changes also in monitor.c. Did you found some issues in your
testing?
May I asked what is a difference between sysv initscript and upstart?
I am fine with conditional including especially if you would use
the same script in downstream(ubuntu) :-)
LS
The changes in monitor.c should be only positional, because the previous
patch was meant to be applied on 1.14.1. I used fedora 24 to prepare
this patch and had no issues with the tests (luckily!).
Regarding the upstart script, I thought it was requested to be included
in the patch, my fault. In ubuntu we require an apparmor profile load,
so it wouldn't make any sense to include it upstream, but thanks anyway :)
Should we stick to the previous patch, then?
Thanks,
Victor