URL:
https://github.com/SSSD/sssd/pull/94
Title: #94: Enable {socket,dbus}-activation for responders
fidencio commented:
"""
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.
> *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-266506811