URL: https://github.com/SSSD/sssd/pull/94
Title: #94: Enable {socket,dbus}-activation for responders
fidencio commented:
"""
On Mon, Dec 12, 2016 at 7:13 PM, Fabiano Fidêncio <fiden...@redhat.com>
wrote:
>
>
> On Mon, Dec 12, 2016 at 2:26 PM, Fabiano Fidêncio <fiden...@redhat.com>
> wrote:
>
>>
>>
>> On Mon, Dec 12, 2016 at 1:34 PM, Pavel Březina <notificati...@github.com>
>> wrote:
>>
>>> *Unit files*
>>> I wonder if the user and group in unit files should take
>>> --with-sssd-user value from configure.
>>>
>>
>> Yeah, it seems the way to go! Jakub also pointed it out during his review.
>>
>>
>>> *MAN: Mention that the services' list is optional*
>>> It should also say that it is socket activated if it is missing.
>>>
>>
>> Right!
>>
>>
>>> *sbus timeout*
>>>
>>> struct sbus_service_timeout {
>>> errno_t (*setup_timeout_fn)(void *);
>>> errno_t (*reset_timeout_fn)(void *);
>>> void *timeout_data;
>>> };
>>>
>>> setup_timeout_fn is not used in sbus, it is used in monitor code so it
>>> should not be part of sbus. I agree to setup the idle timeout in monitor
>>> when we know that the service was bus activated. But I still don't think we
>>> need to expose this logic in sbus. Pass the function pointer to the monitor
>>> (although I think it can be hardcoded) and only pointer to time_t of last
>>> request time to sbus. Also I would prefer to delete
>>> reset_responder_idle_timer and only set rctx->last_request_time
>>> directly without all those checks and a debug message. I believe it will be
>>> more simple and those checks are not really necessary (it doesn't matter if
>>> the timer is set or not, we still can remember last request time).
>>>
>>
>> Hmm. Okay, let me think a little bit about it and do some tests.
>>
>
After doing some changes in the code I have to say it doesn't look cleaner
at all.
>
>>
>>> *sbus_destructor*
>>> Destructors with talloc can be typed directly and still keep type
>>> checking with gcc:
>>>
>>> static int sbus_connection_destructor(void *ctx)
>>> -> static int sbus_connection_destructor(struct sbus_connection *conn)
>>>
>>>
>> Right.
>>
>
> Well, not exactly. GCC will complain in case it's done:
>
> ../src/sbus/sssd_dbus_connection.c: In function ‘sbus_init_connection’:
> ../src/sbus/sssd_dbus_connection.c:181:9: warning: initialization from
> incompatible pointer type [-Wincompatible-pointer-types]
> talloc_set_destructor((TALLOC_CTX *)conn,
> sbus_connection_destructor);
>
> So, if you don't mind I'll avoid the warning and keep it as it is.
>
>
>>
>>
>>> *ensure that timeout is at least 60 seconds*
>>>
>>> + /* Idle timeout set to 0 means that no timeout will be set up to+
>>> * the responder */
>>> + if (rctx->idle_timeout != 0) {
>>> + /* Ensure that the responder timeout is at least sixty seconds */
>>> + if (rctx->idle_timeout < 60) {
>>> + rctx->idle_timeout = 60;
>>> + }
>>> }
>>>
>>> Print a warning here, please.
>>>
>>
>> Not sure if I understand your comment. The reason I'm checking wether the
>> timeout is set to zero or not is because I'm following Lukáš suggestion to
>> disable the timeout in that case.
>> So, yes, I can print a warning message in this case. But I also think
>> that keeping Lukáš suggestion makes sense, right?
>>
>>
>>> *sudo*
>>> Sudo is a little bit special responder. Since it requires some periodic
>>> task in the backend, we decided a long time ago to disable sudo in backend
>>> unless it is explicitly configured, either by putting "services = sudo" or
>>> "sudo_provider = provider". I think this is a good time to treat it the
>>> same as all other responders, i.e. keep it always enabled unless it is
>>> explicitly disabled with sudo_provider = none.
>>>
>> I'm not sure whether I completely understand your suggestion here.
>> You're saying there's some differences between sudo responder and the
>> others. So, your suggestion is to drop these differences as soon as this
>> patch series lands? Or to do these changes within this patch series?
>>
>>
>>> —
>>> You are receiving this because you authored the thread.
>>> Reply to this email directly, view it on GitHub
>>> <https://github.com/SSSD/sssd/pull/94#issuecomment-266420800>, or mute
>>> the thread
>>> <https://github.com/notifications/unsubscribe-auth/AAG4eniHdCjuqepQFbAeSFIcT_fOqjk7ks5rHT9XgaJpZM4K8AJs>
>>> .
>>>
>> Thanks a lot for the review!
>>
>
>
"""
See the full comment at
https://github.com/SSSD/sssd/pull/94#issuecomment-266558041
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
