[SSSD] [PATCHES] python bindings for managing users in local domains

Jakub Hrozek jhrozek at redhat.com
Mon Sep 14 17:57:34 UTC 2009


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

Thank you for the review, new patches attached.

On 09/13/2009 04:20 PM, Simo Sorce wrote:
> On Thu, 2009-09-10 at 23:36 +0200, Jakub Hrozek wrote:
>>> Patch 1:
>>> - mostly good but mem hierarchy between ops_ctx and tools_ctx needs
>> to
>>> be reversed
> 
> ^^^ this is still not addressed, once you do 0001 can be acked.
> 

Sorry, but I'm confused now. During the first review, you said that:
"you should make tctx a child of octx so that talloc_free(octx) will be
enough"
The code now allocates octx first, then passes it as mem_ctx parameter
to setup_db where it is used to create tctx.

>> OK, I pretty much rewrote the whole thing to be tevent_req-stylish
>> with
>> _send _recv etc. I agree that it's way more readable now.
> 
> Much better, we are close, but not there yet.
> 
> 0. It is absolutely paramount that you always have:
> 
> state structure
> _Send()
> step1_done()
> step2_done()
> step3_done()
> ...
> _recv()
> 

(snip very useful explanation)

Hope it is better now, see i.e:

struct user_mod_state
user_mod_send()
user_mod_attr_done()
user_mod_rm_group_done()
user_mod_add_group_done()

> 
> 1. You are trying to use one common state structure for all calls.
> Please don't do that, and especially don't add more function pointer
> (member_dn_fn is really a no go).
> 

All operations now have a separate struct foo_state.

> 
> 2. Really don't like add_to_groups() for a few reasons.
> - it is creating a subreq so it *should* be a _send() function and
> *must* return the subreq and must not set the callback itself.
> - this function and the following should probably be turned into a
> _send(),_done(),_recv() subrequest.

Done.

> - it uses member_dn_fn() please don't do that.

Undone.

> 
> 3. In tevent_req() _done() functions try to never do something like:
>    return foo_function(req);
> - You should either call tevent_req_error() or tevent_req_done(), or
> call a subrequests: subreq = foo_send() and set a callback and then just
> return;
> - a return foo_function(req) is a spy that something is not right (there
> are exceptions, but they must be carefully thought out.
> 
> I know some of the functions (like user_mod_cont()) seem to save a bit
> of duplication and makes the code slightly smaller, but this is an
> optimization that kills readability and doesn't really save much.
> Readability is much more important when it comes to tevent_req stuff.
> 

No more user_mod_cont in the current patchset

>>> Patch 3:
>>> - looks sane but I'd like a second look from one of ours python
>> resident
>>> experts
>>>
>>
>> CC-ed John.
> 
> I see John replied to that part, I'll let you address the concerns he
> raised for the 3rd patch.
> 
> Simo.
> 

John's concerns should be addressed in the attached patch.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkquhA4ACgkQHsardTLnvCVoQACgwuClflP1QrckaZQdBrl8WUEn
kNsAmwYB5Af85nUvG0U9iEn61sP4nn5+
=YhdE
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Refactor-tools-code.patch
Type: text/x-patch
Size: 35784 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090914/436226a5/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Decouple-synchronous-sysdb-interface-from-tools.patch
Type: text/x-patch
Size: 107534 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090914/436226a5/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Provide-python-bindings-for-sysdb.patch
Type: text/x-patch
Size: 51554 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090914/436226a5/attachment-0002.bin>


More information about the sssd-devel mailing list