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>

> Hi,
> if (ret != EOK) {
>     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

> 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


> .
> 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 
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

Reply via email to