URL: https://github.com/SSSD/sssd/pull/94 Title: #94: Enable {socket,dbus}-activation for responders
fidencio commented: """ On Thu, Dec 1, 2016 at 1:03 PM, Pavel Březina <notificati...@github.com> wrote: > Hi, > > if (ret != EOK) { > #ifdef HAVE_SYSTEMD > if (ret != ENOENT) > #endif > { > DEBUG(SSSDBG_FATAL_FAILURE, "No services configured!\n"); > return EINVAL; > } > } > > can you separate above to: > > #ifdef HAVE_SYSTEMDif (ret != EOK && ret != ENOENT) { > DEBUG(SSSDBG_FATAL_FAILURE, "No services configured!\n"); > return EINVAL; > } > #else > ... > #endif > > Okay, > We should also amend services option man page to describe what happens if > a service is not listed there, that they are automatically activated when > needed > Okay. > . > > Now I have few more comments to the timeouts. > > 1. I believe you can remove "RESPONDER: Shutdown > {dbus,socket}-activated responders in case they're idle" in favor of > "Change the timeout logic". > > The "Change timeout logic" commit is a leftover that must have been squashed to "RESPONDER: Shutdown {dbus,socket}-activated responders in case they're idle". Sorry for messing it up. Squashing it there is okay for you? > > 1. Please use the same logic in sbus code. I think you just want to > pass a pointer to last_request_time into sbus (to remove the need for > function pointers) and let the sbus code update it when appropriate even > for our private communication (in other responders). Because if a > communication is happening between responder and provider, the responder is > still not idle (it may be awaiting reply from data provider so it can be > send to the client). > > Okay, I can pass just the pointer to the last_request_time var into sbus. Please, here I need a bit more pointers in order to be sure I understand your suggestion. Please, correct me if I'm wrong, it's updating the last_request_time even for our private communication, no? Are you talking specifically about IFP provider or the others? Because the others have their last_request_time() updated everytime something goes through their sockets and, as far as I understand, it should be enough, right? If not, why not? > > 1. Resetting the timeout when sbus signal is received is not enough. > You want to reset it everytime any communication on the bus is happening. I > actually think that signals are the only communication that we can skip for > two reasons (One - we don't use any signals except NameOwnerChanged, Second > - those are asynchronous events that do not require any reply). I think you > want to reset the time in sbus_message_handler when you got a valid > handler (interface and method combination. The you don't need the iface > validator. > > Hmmm. I see your point and it makes sense. But I have one question ... We do receive signals from org.freedesktop.sssd.infopipe.* and they should be treated, right? So we can't ignore it completely. In case those signals end up in sbus_message_handler() as well, then I agree with you. Is that the case? > > > — > 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-264156064>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAG4ev7GREjEnsOvsuU94yxTOH6T1zj5ks5rDreogaJpZM4K8AJs> > . > Thanks a lot for the review, Pavel. """ See the full comment at https://github.com/SSSD/sssd/pull/94#issuecomment-264162692
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org