[PATCHES] python bindings for managing users in local domains

[Jakub Hrozek]

-----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 SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqNx4UACgkQHsardTLnvCXWSQCgyVRBOCQOPr1Wltg6Kv9/7Bxk
> C9AAmwcvhQ2VxUeG4uMklgJT19cTzw6L
> =l7DI
> -----END PGP SIGNATURE-----

[Jakub Hrozek]

-----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 SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqOlS4ACgkQHsardTLnvCW5jwCgvkmlhYAzZ4ErzG1roQ3xPh8w
> krQAoL7ENZNy+lHSaEN+xuIaCqgYBbHs
> =3jOe
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----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(a)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 SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqOnDYACgkQeiVVYja6o6MD4QCfWbUPNwisPMIwjH9eDQkwif6O
> G8UAnRlZIgtO0GbZ+OnQc91nR6MFuOBc
> =bsXl
> -----END PGP SIGNATURE-----

[Jakub Hrozek]

-----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 SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqTnwgACgkQHsardTLnvCUA0wCghXUO8v+NfKh+AxxFipO0fUCf
> LX4An1N/+TFn1Vg/7Fbvq+etQ6Ht8hmE
> =i7dd
> -----END PGP SIGNATURE-----

[Jakub Hrozek]

-----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 SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqT4M4ACgkQHsardTLnvCWjagCg7CIAG9jDRK9SfjIazgEtUZDA
> 0CkAoNrh9Fu16wXJvAfJssbV3FfSgT/2
> =DajG
> -----END PGP SIGNATURE-----

[Jakub Hrozek]

-----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
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqU+gUACgkQHsardTLnvCVMxwCgxe+JhLzAsvTrnH24WjqzM8u5
> JWsAn2C0cQuEjxIZzTiZKcBQ8kkenbHl
> =kTaD
> -----END PGP SIGNATURE-----

[Simo Sorce]

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.

-- 
Simo Sorce * Red Hat, Inc * New York

[Jakub Hrozek]

-----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 SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqpcVwACgkQHsardTLnvCUUTgCgyBwqkDnhrFb+Z4a6NHdM9h4k
> zScAoNwiXi7byuGuXyGg6VeNX82KpMuR
> =8NYQ
> -----END PGP SIGNATURE-----

[Jakub Hrozek]

-----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.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqpc+IACgkQHsardTLnvCX+6gCgxCh12TjlJ4fslsWHxZeeVNER
> 9Z8An03PlavFWu4u5PvH4bRadh/L4Oc0
> =ziYk
> -----END PGP SIGNATURE-----

[John Dennis]

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.

-- 
John Dennis <jdennis(a)redhat.com&gt;

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

[Jakub Hrozek]

-----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
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkquiX4ACgkQHsardTLnvCX62wCdEI+8hm7x0ElI2oL/HKWHhRNe
> By8AoMKtPGQJR68JRu9cDUbA9wpt2HjX
> =UILg
> -----END PGP SIGNATURE-----

[Simo Sorce]

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.

-- 
Simo Sorce * Red Hat, Inc * New York

[Jakub Hrozek]

-----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-----

[Simo Sorce]

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.

-- 
Simo Sorce * Red Hat, Inc * New York

[Jakub Hrozek]

-----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
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqurJ8ACgkQHsardTLnvCWUdQCg2z3+O2v5M1rrVKSdYY4WfH3+
> r4UAoInwT3pZeljigRN5al0kF2QP+oSB
> =mUza
> -----END PGP SIGNATURE-----

[Simo Sorce]

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.

-- 
Simo Sorce * Red Hat, Inc * New York

[Jakub Hrozek]

-----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 SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqutUgACgkQHsardTLnvCUAXgCgjBiAzhSgOMojlY6TbCO6jlT1
> h/gAn1J72sPPTirB+lHim8SVh6Wi1v8M
> =R/Bd
> -----END PGP SIGNATURE-----

[Jakub Hrozek]

-----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
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqzjYUACgkQHsardTLnvCX2UQCfYyPL1SVp3Y2HTg2fC6tUu/sf
> MnoAn3FnLvUCoAGSoRBOp1SnQb61AO8v
> =iYfS
> -----END PGP SIGNATURE-----

[John Dennis]

The python binding (pysss.c) looks good to me.

ACK for pysss.c


-- 
John Dennis <jdennis(a)redhat.com&gt;

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

[Stephen Gallagher]

-----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(a)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 SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqzmK8ACgkQeiVVYja6o6Pw/QCeIL8g/wiWeldCHY9m1w/mc42P
> vyIAn2o3JcPA/EHJADp9BljNfYd2VjVs
> =91fg
> -----END PGP SIGNATURE-----

[Jakub Hrozek]

-----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 SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkqz7aAACgkQHsardTLnvCX8UQCeKnE2clHRkyL4OzkJjcNNIHKg
> hVwAn2UljbsxbCmKctbCZ245ko6dd36C
> =unKE
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----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(a)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 SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkq3VPwACgkQeiVVYja6o6MvYQCghGcfs2A+epmIDlUCokaPKShL
> QqoAnjDsEP1AFCx1YurnbIl5BzKgWbO8
> =euoD
> -----END PGP SIGNATURE-----

[Jakub Hrozek]

-----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 SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkq3digACgkQHsardTLnvCU8SQCeIzd4XT92ox97P+du0Z10v8Io
> +o8An1XSDgtzYEWyaHBBJtJmJme4wfmR
> =WGU0
> -----END PGP SIGNATURE-----

[Stephen Gallagher]

-----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(a)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 SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkq3j2IACgkQeiVVYja6o6OY1gCfejXPjUAEhwMHe2q50FvmEuYm
> AE0An2fl/ovVkeHy1GRQMGii3k8IVxjw
> =hXK0
> -----END PGP SIGNATURE-----