-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Attached are three patches.
[PATCH 1/3] Refactor tools code Refactores tools code while still retaining the original structure. Move parameter parsing in tools before attempting to do anything that might fail - so that we have debug_level set correctly for potential error messages. That allows printing the --help and --usage messages without being root.
[PATCH 2/3] Decouple synchronous sysdb interface from tools Instead of working directly with async code in tools, create synchronous wrappers that could be used by tools and python bindings.
[PATCH 3/3] Provide python bindings for sysdb Implement a set of python bindings for the sysdb with feature set similar to what is available in the tools. The primary consumers would be applications like system-config-users.
These three patches resolve tickets 102, 87 and 95.
Jakub
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 08/21/2009 12:01 AM, Jakub Hrozek wrote:
Attached are three patches.
[PATCH 1/3] Refactor tools code Refactores tools code while still retaining the original structure. Move parameter parsing in tools before attempting to do anything that might fail - so that we have debug_level set correctly for potential error messages. That allows printing the --help and --usage messages without being root.
[PATCH 2/3] Decouple synchronous sysdb interface from tools Instead of working directly with async code in tools, create synchronous wrappers that could be used by tools and python bindings.
[PATCH 3/3] Provide python bindings for sysdb Implement a set of python bindings for the sysdb with feature set similar to what is available in the tools. The primary consumers would be applications like system-config-users.
These three patches resolve tickets 102, 87 and 95.
Jakub
Rebased on top of a later fix.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 08/20/2009 06:01 PM, Jakub Hrozek wrote:
Attached are three patches.
[PATCH 1/3] Refactor tools code Refactores tools code while still retaining the original structure. Move parameter parsing in tools before attempting to do anything that might fail - so that we have debug_level set correctly for potential error messages. That allows printing the --help and --usage messages without being root.
[PATCH 2/3] Decouple synchronous sysdb interface from tools Instead of working directly with async code in tools, create synchronous wrappers that could be used by tools and python bindings.
[PATCH 3/3] Provide python bindings for sysdb Implement a set of python bindings for the sysdb with feature set similar to what is available in the tools. The primary consumers would be applications like system-config-users.
These three patches resolve tickets 102, 87 and 95.
Jakub
Patch 0001: Nack: domain_lookup(): Please check that ops is not NULL before using it, just to be safe. Also, the second return ret doesn't make sense. It's not reset anywhere, so it can only be EOK. I rather assume that's not what you were going for here. Also, why are you checking if ops->uid is nonzero in that if() statement? I think just checking if(ops->domain && ops->domain != dom) is sufficient (since get_domain_by_id() has already established the validity of ops->uid) Finally, please return EOK at the end, rather than 0 (for clarity)
run_command(): Please explicitly check if (ret != EXIT_SUCCESS) rather than just if (ret).
Patch 0002: Nack: To perform a safe synchronous transaction, it would be best to guarantee a private event context, rather than use data->ctx->ev. synchronous_transaction() should set up a new tevent_context for the duration of the transaction. (I'm aware that the tools themselves are more-or-less synchronous, but if we opt to use this interface outside the tools, we can't guarantee that the event context is safe to use)
parse_groups(): Remove the FIXME about strtok(). We're not using it anymore.
When moving run_command(), please ensure that you make the same explicit check mentioned in Patch 0001
Patch 0003: Nack. I think we probably want the python bindings built by default, with the option to turn them off. Unless you can make a good argument against it.
In python.m4, you do not properly quote the output strings for AC_MSG_RESULT and AC_MSG_ERROR
Implementation of the bindings themselves look good to me.
- ------------------------------------------------------------------------
_______________________________________________ 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/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 08/21/2009 03:08 PM, Stephen Gallagher wrote:
Patch 0001: Nack: domain_lookup(): Please check that ops is not NULL before using it, just to be safe.
done
Also, the second return ret doesn't make sense. It's not reset anywhere, so it can only be EOK. I rather assume that's not what you were going for here.
Thanks, that was a bug. Fixed to return EINVAL.
Also, why are you checking if ops->uid is nonzero in that if() statement? I think just checking if(ops->domain && ops->domain != dom) is sufficient (since get_domain_by_id() has already established the validity of ops->uid)
Yes and no. Checking for ops->uid is correct but obfuscated the code, I changed it to more readable if(dom && ops->domain && ops->domain != dom) as you only want to assert ops->domain==dom if domain was given in form of FQDN (so it's in ops->domain) and uid (so it's in dom after get_domain_by_id()). Hope it makes more sense now.
Finally, please return EOK at the end, rather than 0 (for clarity)
done
run_command(): Please explicitly check if (ret != EXIT_SUCCESS) rather than just if (ret).
done
Patch 0002: Nack: To perform a safe synchronous transaction, it would be best to guarantee a private event context, rather than use data->ctx->ev. synchronous_transaction() should set up a new tevent_context for the duration of the transaction. (I'm aware that the tools themselves are more-or-less synchronous, but if we opt to use this interface outside the tools, we can't guarantee that the event context is safe to use)
OK, done. I've separated ops_ctx from being directly dependent on any event context, its purpose is only to carry data now. synchronous_transaction() now sets up a new event context.
parse_groups(): Remove the FIXME about strtok(). We're not using it anymore.
When moving run_command(), please ensure that you make the same explicit check mentioned in Patch 0001
done and done.
Patch 0003: Nack. I think we probably want the python bindings built by default, with the option to turn them off. Unless you can make a good argument against it.
OK, the attached version builds them by default. The reason I selected no by default was that I wasn't sure about packaging - do we want a separate sssd-python subpackage?
In python.m4, you do not properly quote the output strings for AC_MSG_RESULT and AC_MSG_ERROR
Thanks, fixed
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 08/25/2009 10:21 AM, Jakub Hrozek wrote:
Also, why are you checking if ops->uid is nonzero in that if() statement? I think just checking if(ops->domain && ops->domain != dom) is sufficient (since get_domain_by_id() has already established the validity of ops->uid)
Yes and no. Checking for ops->uid is correct but obfuscated the code, I changed it to more readable if(dom && ops->domain && ops->domain != dom) as you only want to assert ops->domain==dom if domain was given in form of FQDN (so it's in ops->domain) and uid (so it's in dom after get_domain_by_id()). Hope it makes more sense now.
Sorry, actually the first version was correct. I forgot that you can specify uid and still not get a valid domain back -- which is when you select both username@DOMAIN and an UID that is outside all domains.
I also took the liberty of chanking the get_domain_by_id() logic to fix ticket #112, too. I tested this version extensively with all kinds of weird domain-uid combinations and it should be finally OK. Sorry for the noise.
All three patches attached for clarity, but in reality only 0001 changed.
Jakub
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 08/25/2009 03:02 PM, Jakub Hrozek wrote:
On 08/25/2009 10:21 AM, Jakub Hrozek wrote:
> Also, why are you checking if ops->uid is > nonzero in that if() statement? I think just checking > if(ops->domain && ops->domain != dom) is sufficient (since > get_domain_by_id() has already established the validity of ops->uid)
Yes and no. Checking for ops->uid is correct but obfuscated the code, I changed it to more readable if(dom && ops->domain && ops->domain != dom) as you only want to assert ops->domain==dom if domain was given in form of FQDN (so it's in ops->domain) and uid (so it's in dom after get_domain_by_id()). Hope it makes more sense now.
Sorry, actually the first version was correct. I forgot that you can specify uid and still not get a valid domain back -- which is when you select both username@DOMAIN and an UID that is outside all domains.
I also took the liberty of chanking the get_domain_by_id() logic to fix ticket #112, too. I tested this version extensively with all kinds of weird domain-uid combinations and it should be finally OK. Sorry for the noise.
All three patches attached for clarity, but in reality only 0001 changed.
Jakub
One more change, while working on #138 I realized that the copyright headers were incorrect -- I had a GPLv2 header in my personal c source file template, while the rest of the project uses GPLv3.
I also squashed in a fix for a typo in user-visible error message (fixes #136) into patch #1.
Jakub
On Wed, 2009-08-26 at 11:01 +0200, Jakub Hrozek wrote:
One more change, while working on #138 I realized that the copyright headers were incorrect -- I had a GPLv2 header in my personal c source file template, while the rest of the project uses GPLv3.
I also squashed in a fix for a typo in user-visible error message (fixes #136) into patch #1.
I have given some more detail feedback on IRC, but here for the records.
Patch 1: - mostly good but mem hierarchy between ops_ctx and tools_ctx needs to be reversed - also some cases where codyng style is not followed (missing space between 'if' and '('
Patch 2: NACK - do not create tools/common/ keep all in tools/ until we have a lot's of files to get out of the way - main NACK point is that tevent_req async coding style has not been followed in the sync wrappers, details explained on IRC - also do the transaction as a separate operation and simply pass in the handle to the sync ones, so that multiple sync operations can be linked into a single transaction.
Patch 3: - looks sane but I'd like a second look from one of ours python resident experts
Simo.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 08/28/2009 05:49 PM, Simo Sorce wrote:
On Wed, 2009-08-26 at 11:01 +0200, Jakub Hrozek wrote:
One more change, while working on #138 I realized that the copyright headers were incorrect -- I had a GPLv2 header in my personal c source file template, while the rest of the project uses GPLv3.
I also squashed in a fix for a typo in user-visible error message (fixes #136) into patch #1.
I have given some more detail feedback on IRC, but here for the records.
Patch 1:
- mostly good but mem hierarchy between ops_ctx and tools_ctx needs to
be reversed
- also some cases where codyng style is not followed (missing space
between 'if' and '('
Should be done. Note that the patch is different than the original one b/c of the removal of shadow-utils fallback.
Patch 2: NACK
- do not create tools/common/ keep all in tools/ until we have a lot's
of files to get out of the way
done
- main NACK point is that tevent_req async coding style has not been
followed in the sync wrappers, details explained on IRC
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.
- also do the transaction as a separate operation and simply pass in the
handle to the sync ones, so that multiple sync operations can be linked into a single transaction.
Transactions are now done outside the "sync" module, that is, either in the tools or in the python bindings module.
Patch 3:
- looks sane but I'd like a second look from one of ours python resident
experts
CC-ed John.
Thanks for the review!
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/10/2009 11:36 PM, Jakub Hrozek wrote:
Patch 3:
- looks sane but I'd like a second look from one of ours python resident
experts
CC-ed John.
Thanks for the review!
Apparently the configure.ac change conflicts with one of recent changes from another patch, rebased version of the bindings patch is attached.
Sorry for the noise.
I've reviewed the python binding for sss. It looks pretty good Jakub. I only have a few comments and most of these are style issues as opposed to a coding or logic flaw.
* In PyErr_SetSssErrorWithMessage() you don't need to call the PyExc_IOError constructor, instead use Py_BuildValue and use the returned tuple as the exception value. To be honest I'm not sure what it means to set the value of an exception to another exception which is what it appears to be what you're doing.
* Some of the function parameters are python objects obtained using the "O" format specifier for PyArg_ParseTuple. However in each instance they can't be any python object, rather they must be List objects. You do check the object type when you pass the object to PyList_AsStringList, but I'd rather see the validation of the object type happen earlier and follow existing Python convention of throwing a type exception directly from PyArg_ParseTuple. This can easily be done by specifying the format specifier as "O!" rather than "O". When you do this you must add an additional argument to PyArg_ParseTuple which is a pointer to the object type. For example
PyArg_ParseTuple(args, "O!", &PyList_Type, &py_list)
This way the error is caught before any processing is done in the function and it then become explicitly obvious to someone reading the code what type of object the parameter must be for correct operation. It also means the error message will exactly match all other parameter type errors thrown by python (consistency has a value).
* You're using two different memory management libraries, talloc and Python's. It does appear you're keeping the usage of the two separate which is critical (however I didn't carefully check all the usage). However mixing two different memory management libraries in one body of code makes me nervous, the opportunity to call the wrong library function on a pointer with disastrous consequences is always lurking. I presume you're using talloc because sss needs it, but if you can avoid mixing two different memory management libraries in one body of code you'll eliminate one potential source of obscure bugs.
* Documentation strings. I'd rather see the docmentation strings appear next to the functions they are documenting, not in a far off place in the file. That way when someone is reading the code they can see the documentation for the function they're reading and more importantly they can see the parameter lists and correlate that with argument extraction (PyParseTuple). PyDoc_STRVAR() is a common convention.
* Documentation format. In IPA we've decided to use reStructured text as the markup (see http://freeipa.com/page/Python_Coding_Style#Python_API_documentation). Your docstrings look like they're in epydoc markup.
Overall the code looks good, you've done a good job.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/11/2009 10:56 PM, John Dennis wrote:
I've reviewed the python binding for sss. It looks pretty good Jakub. I only have a few comments and most of these are style issues as opposed to a coding or logic flaw.
Thank you for the review, the comments should be fixed in patch attached to Simo's review of the other two patches.
- In PyErr_SetSssErrorWithMessage() you don't need to call the
PyExc_IOError constructor, instead use Py_BuildValue and use the returned tuple as the exception value. To be honest I'm not sure what it means to set the value of an exception to another exception which is what it appears to be what you're doing.
Thanks, fixed
- Some of the function parameters are python objects obtained using the
"O" format specifier for PyArg_ParseTuple. However in each instance they can't be any python object, rather they must be List objects. You do check the object type when you pass the object to PyList_AsStringList, but I'd rather see the validation of the object type happen earlier and follow existing Python convention of throwing a type exception directly from PyArg_ParseTuple. This can easily be done by specifying the format specifier as "O!" rather than "O". When you do this you must add an additional argument to PyArg_ParseTuple which is a pointer to the object type. For example
PyArg_ParseTuple(args, "O!", &PyList_Type, &py_list)
This way the error is caught before any processing is done in the function and it then become explicitly obvious to someone reading the code what type of object the parameter must be for correct operation. It also means the error message will exactly match all other parameter type errors thrown by python (consistency has a value).
Fixed.
- You're using two different memory management libraries, talloc and
Python's. It does appear you're keeping the usage of the two separate which is critical (however I didn't carefully check all the usage). However mixing two different memory management libraries in one body of code makes me nervous, the opportunity to call the wrong library function on a pointer with disastrous consequences is always lurking. I presume you're using talloc because sss needs it, but if you can avoid mixing two different memory management libraries in one body of code you'll eliminate one potential source of obscure bugs.
I know it's not ideal, but AFAIK there is no way to use tevent/confdb et al. without talloc.
- Documentation strings. I'd rather see the docmentation strings appear
next to the functions they are documenting, not in a far off place in the file. That way when someone is reading the code they can see the documentation for the function they're reading and more importantly they can see the parameter lists and correlate that with argument extraction (PyParseTuple). PyDoc_STRVAR() is a common convention.
Thanks, it appears that the macro is underdocumented on docs.python.org, so I had no idea it even existed.
- Documentation format. In IPA we've decided to use reStructured text as
the markup (see http://freeipa.com/page/Python_Coding_Style#Python_API_documentation). Your docstrings look like they're in epydoc markup.
Yes, I'm used to epydoc, so I actually forgot we should be using RST. I changed the comments to RST, but it was actually my first time using it, so maybe I didn't use some tags I should be or vice versa. Tested rendering the docs with "epydoc --docformat=restructuredtext pysss.local", looks sane.
Jakub
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.
- also some cases where codyng style is not followed (missing space
between 'if' and '('
Should be done. Note that the patch is different than the original one b/c of the removal of shadow-utils fallback.
The rest looks ok.
Patch 2: NACK
- do not create tools/common/ keep all in tools/ until we have a
lot's
of files to get out of the way
done
- main NACK point is that tevent_req async coding style has not been
followed in the sync wrappers, details explained on IRC
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()
In this order, if you have to jump around to find out any step the code becomes very soon unreadable and it is extremely easy to make mistakes. This code is already somewhat harder than usual to debug (need to set a lof of break points to follow the flow) so it is imperative that you can read it as a single set of consecutive functions. The importance of this is immediately evident whan you have to A) review some else's code (like I am doing now), B) you have to modify come code. Especially important is to share only utility functions but never functions that create tevent_req requests.
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).
When you share the same state structure a typo later in the code where you set (for example) a wrong callback (say a copy&paste error where you call the xx_users_recv() instead of the xx_groups_recv() will give back no error. If you use different state functions though, you can use talloc_get_type() to check for the type, and NULL will be returned if the type is wrong resulting in a segfault the first time you test it.
Also remember that tevent_req_create() does not zero out the memory (on purpose it's not a bug), so anything you do not initialize explicitly in the _send() function is random.
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. - it uses member_dn_fn() please don't do that.
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.
- also do the transaction as a separate operation and simply pass in
the
handle to the sync ones, so that multiple sync operations can be
linked
into a single transaction.
Transactions are now done outside the "sync" module, that is, either in the tools or in the python bindings module.
good.
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.
-----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.
- 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()
- 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.
- 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.
- 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.
On Mon, 2009-09-14 at 19:57 +0200, Jakub Hrozek wrote:
-----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.
this is the code I see for the function init_sss_tools()
+ octx = talloc_zero(tctx, struct ops_ctx); + if (octx == NULL) { + DEBUG(1, ("Could not allocate memory for data context\n")); + ERROR("Out of memory\n"); + ret = ENOMEM; + goto fini; + } + octx->ctx = tctx; + + *_octx = octx;
This means that the 'octx' memory context is the child of 'tctx'. Needs to be the other way around otherwise if you free octx, tctx will not be automatically freed.
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()
[..]
All operations now have a separate struct foo_state.
_send(),_done(),_recv() subrequest.
Done.
[..]
- it uses member_dn_fn() please don't do that.
Undone.
[..]
No more user_mod_cont in the current patchset
Awesome, this latest code looks exactly as I was thinking it should be. I'll test it asap.
Patch 3:
- looks sane but I'd like a second look from one of ours python
resident
experts
I see John replied to that part, I'll let you address the concerns he raised for the 3rd patch.
John's concerns should be addressed in the attached patch.
I'll let John comment on it
Simo.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/14/2009 08:53 PM, Simo Sorce wrote:
this is the code I see for the function init_sss_tools()
- octx = talloc_zero(tctx, struct ops_ctx);
- if (octx == NULL) {
DEBUG(1, ("Could not allocate memory for data context\n"));ERROR("Out of memory\n");ret = ENOMEM;goto fini;- }
- octx->ctx = tctx;
- *_octx = octx;
This means that the 'octx' memory context is the child of 'tctx'. Needs to be the other way around otherwise if you free octx, tctx will not be automatically freed.
Ahh, I know what happened. I fixed the behavior after the first review, but squashed the fix into the second patch (see the changes in tools_util.c), not the first one. And because I was looking at code my HEAD, I saw only the fixed behavior..
Should I redo this? The patches will most probably be applied together, but maybe it's better if every patch is self-contained..
Jakub
On Mon, 2009-09-14 at 22:51 +0200, Jakub Hrozek wrote:
Should I redo this? The patches will most probably be applied together, but maybe it's better if every patch is self-contained..
If it will not take you too much time it would be nice to have them straightened :)
If you think it will take too long to fix, just let me know and I'll just push them together.
Simo.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/14/2009 10:54 PM, Simo Sorce wrote:
If it will not take you too much time it would be nice to have them straightened :)
If you think it will take too long to fix, just let me know and I'll just push them together.
Simo.
git add -i to the rescue!
The attached patches also fix a conflict in Makefile.am between them & one of the recent kerberos patches.
Jakub
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/14/2009 11:27 PM, Jakub Hrozek wrote:
The attached patches also fix a conflict in Makefile.am between them & one of the recent kerberos patches.
Jakub
Resending patch 3/3 as it conflicted in sssd.spec.in
Jakub
The python binding (pysss.c) looks good to me.
ACK for pysss.c
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/14/2009 05:27 PM, Jakub Hrozek wrote:
On 09/14/2009 10:54 PM, Simo Sorce wrote:
If it will not take you too much time it would be nice to have them straightened :)
If you think it will take too long to fix, just let me know and I'll just push them together.
Simo.
git add -i to the rescue!
The attached patches also fix a conflict in Makefile.am between them & one of the recent kerberos patches.
Jakub
Patch 0001: Ack Patch 0002:
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.
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?
Patch 0003: Left to John to review
- ------------------------------------------------------------------------
_______________________________________________ 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/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
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
-----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/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/21/2009 12:27 PM, Stephen Gallagher wrote:
Fix that talloc parentage and this will be an Ack.
Attached are all three patches for convenience.
Jakub
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 09/21/2009 08:48 AM, Jakub Hrozek wrote:
On 09/21/2009 12:27 PM, Stephen Gallagher wrote:
Fix that talloc parentage and this will be an Ack.
Attached are all three patches for convenience.
Jakub
Ack and pushed
- ------------------------------------------------------------------------
_______________________________________________ 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/
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
John Dennis -
Simo Sorce -
Stephen Gallagher