URL:
https://github.com/SSSD/sssd/pull/94
Title: #94: Enable {socket,dbus}-activation for responders
fidencio commented:
"""
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.
*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?
Thanks a lot for the review!
"""
See the full comment at
https://github.com/SSSD/sssd/pull/94#issuecomment-266430423