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 <[email protected]> wrote: > > > On Mon, Dec 12, 2016 at 1:34 PM, Pavel Březina <[email protected]> > 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/AAG4eniHdCjuqepQFbAeSFIcT_fOqjk7ks5rHT9XgaJpZM4K8AJs> >> . >> > Thanks a lot for the review! > """ See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-266506811
_______________________________________________ sssd-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
