[SSSD] function names in multi-task scenarios

Stephen Gallagher sgallagh at redhat.com
Fri Jan 17 12:46:48 UTC 2014


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/16/2014 04:25 PM, Jakub Hrozek wrote:
> On Wed, Jan 15, 2014 at 05:20:58PM -0500, Yassir Elley wrote:
>> In looking through the sssd code, there are instances where an
>> asynchronous operation requires multiple tasks to be done in
>> series (not to be confused with a parent request with multiple
>> child requests). An example of a series of such tasks would look
>> like: * send() * task1() * task2() * done()
> 
> One word of warning about the tevent_req style. Some old code in
> the SSSD predates this style and both the functions and the
> accompanying structure can be named differently (the structure
> typically wouldn't be a *_state but rather a *_ctx).
> 
>> 
>> While one would expect that the tasks would be named so as to
>> describe their behavior, they are typically named as completing a
>> previous task, giving no indication what the task is going to do.
>> This can be confusing when reading the logs. For example, if the
>> problem is that kinit failed, an entry in the log might have the
>> function name "sdap_cli_rootdse_done", which doesn't help
>> describe the kinit problem (the call stack below shows how
>> "sdap_cli_rootdse_done" calls "sdap_cli_kinit_step").
>> 
>> In the following call stack, I have used the "bad name"
>> annotation for those function names that don't describe the
>> function behavior. I have also suggested using a more descriptive
>> name that describes the functions behavior (each using a
>> <suffix>). I'm not sure what the <suffix> should be; maybe "task"
>> or "phase" or something.
>> 
>> sdap_cli_connect_send sdap_cli_resolve_next sdap_cli_resolve_done
>> bad name; sdap_cli_connect_<suffix> would be better (maybe
>> sdap_cli_connect_task) sdap_connect_send sdap_cli_connect_done
>> bad_name; sdap_cli_rootdse_<suffix> would be better 
>> sdap_cli_rootdse_step sdap_cli_rootdse_done            bad_name;
>> sdap_cli_kinit_<suffix> would be better sdap_cli_kinit_step 
>> sdap_cli_kinit_done              bad_name; sdap_cli_auth_<suffix>
>> would be better sdap_cli_auth_step sdap_cli_auth_done
>> bad_name; sdap_cli_connect_done would be better tevent_req_done
>> 
>> In addition to being non-descriptive, the "done" suffix is
>> currently used in two different ways, which can be confusing: *
>> as shown above, "done" is used for a series of intermediate
>> tasks, whose names are based on previous tasks * in a simpler
>> scenario (with no intermediate tasks), "done" is used to signify
>> the final task (which makes sense); for example:
>> 
>> sdap_id_op_connect_send sdap_id_op_connect_step 
>> sdap_id_op_connect_done
>> 
>> To resolve both issues, I propose we improve our naming
>> conventions by: * coming to consensus on a suffix for a series of
>> intermediate tasks
> 
> I'm not sure we can force a common suffix everywhere. I think this 
> should be one of 'use your best judgement' cases. Maybe it would
> make sense to use past tense when the function ends with a verb
> (ie sdap_cli_connected) ?
> 
>> * declaring "done" to be used only for a final task (whether in a
>> basic scenario, or a scenario with multiple intermediate tasks)
>> 
>> What do others think?
> 
> I think the most important part of the tevent_req style is to have
> a common prefix and use it for all of _send/_done/_recv:
> 
> struct $prefix_state { };
> 
> $prefix_send(); $prefix_done(); $prefix_recv();
> 
> The intermediate 'steps' I'm not sure about, I don't think that
> needs to be formally codified.

This came out of a conversation that Yassir and I had the other day.
One of the pieces that I see as being problematic in our style is just
as he describes it: we're not following best-practice naming for our
functions. The best practice is that each intermediate step in the
execution should be named after what it is doing, not what it just
finished doing. It makes no sense when looking at the debug logs to
get an error reported in sdap_cli_rootdse_done() when the real action
taking place is that we're attempting to perform a GSSAPI bind.

I agree with Yassir that I'd like to see a behavior more like the
above, where we would have:

$prefix_send()
$prefix_$description1_task()
$prefix_$description2_task()
...
$prefix_done()

And then as before, retrieve results with
$prefix_recv()


This pattern would allow us to be more consistent, while also seeing
trivially in the logs what operation we're performing and how far
along we are in it. Yes, this will likely result in very long function
names. I'm willing to accept that (our function names aren't exactly
short right now anyway).

So to take a real-world example that I would be willing to refactor:

Here's the current operation (loops eliminated for brevity):

sdap_access_filter_send():
    Establishes an sdap_id_op and connects to LDAP

sdap_access_filter_connect_done()
    Sends an LDAP base-search request for the user with a specialized
    filter to determine if the user will match it.

sdap_access_filter_get_access_done()
    Checks whether we received zero (denied) or one (granted) as a
    reply.


So in the proposed new pattern, this would become:

sdap_access_filter_send()
sdap_access_filter_perform_filter_check_task()
sdap_access_filter_done()


There's some question in my mind as well as to whether we might want
the _done() function to also be more descriptive. In the above case,
we might instead call it sdap_access_filter_authorization_done() (or
*_authz_done())


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLZJjgACgkQeiVVYja6o6MzeACgm8tgFQ5T/pNZLVtDZ6K31Itk
EPYAnRoBUP+cnI4YdFI54MmqOn0+d5SL
=2aGa
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list