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