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 <fidencio(a)redhat.com>
wrote:
On Mon, Dec 12, 2016 at 2:26 PM, Fabiano Fidêncio <fidencio(a)redhat.com>
wrote:
>
>
> On Mon, Dec 12, 2016 at 1:34 PM, Pavel Březina <notifications(a)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/AAG4eniHdCjuqepQFbAeSFI...
>> .
>>
> Thanks a lot for the review!
>
"""
See the full comment at
https://github.com/SSSD/sssd/pull/94#issuecomment-266558041