-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/18/2009 04:29 PM, Jakub Hrozek wrote:
On 09/18/2009 04:26 PM, Stephen Gallagher wrote:
usermod(), useradd(), etc.: res should be a talloc child of req. (Also, in the current implementation, if user_mod_send returned NULL for ENOMEM, you would leak res)
user_mod_send(), group_mod_send: subreq should be a talloc child of req. If for some reason we cancelled the request for usermod, we want to make sure that its subrequests aren't continuing.
These two I agree with. Thanks for spotting these.
There's a lot of code duplication in user_mod_*. Is it possible to code this in such a way that you only have one common callback that processes the results and kicks off another request if needed?
Yes, but please see Simo's comments[1] to the earlier patches. It seems that there are different views on code readability (even with duplication) versus trying not to duplicate code at all, which is what I tried to do earlier, esp:
<cite> 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. </cite>
So, I very much appreciate the review, but to be honest, I'd like to avoid rewriting the code back and forth until I know what is the approach that would be welcome by all parties.
Thank you for the review, Jakub
[1] https://fedorahosted.org/pipermail/sssd-devel/2009-September/000521.html
You know, put that way, I guess I agree with this approach. I'm just generally averse to having duplicated code because it means multiple places to change if we have to modify it. But if it's a conscious decision made to improve readability, I'm behind it.
Fix that talloc parentage and this will be an Ack.
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
- -- Stephen Gallagher RHCE 804006346421761
Looking to carve out IT costs? www.redhat.com/carveoutcosts/