On 01/07/2016 02:01 PM, Sumit Bose wrote:
On Tue, Jan 05, 2016 at 11:41:45AM +0100, Pavel Březina wrote:
> On 12/18/2015 02:36 PM, Pavel Březina wrote:
>> Hello list,
>> the support of IPA sudo schema is almost complete. The only thing that
>> remains is to finish smart refresh implementation and one patch to
>> reduce code duplication between LDAP and IPA implementation. Then I need
>> to run some tests but I don't expect much troubles here since I tested
>> it a lot during development. I'll finish all of this after my Christmas
>> vacation.
>>
>> The patches are probably too big to be sent as an attachment, so until I
>> complete the last two patches, you can check it on my git repo, branch
>> sudo [1]. I don't really expect anyone to review them during Christmas
>> break, but I thought it's a good thing to present if in case someone
>> will get really bored from all the candies and family visits :-)
>>
>> Happy reviewing.
>>
>> [1]
https://fedorapeople.org/cgit/pbrezina/public_git/sssd.git/log/?h=sudo
>
> Final patches are attached. Happy reviewing.
>
Here are my first comments:
0001:
timeout == 0 is checked twice
Fixed.
+
+ for (state->map_num_attrs = 0;
+ state->map[state->map_num_attrs].opt_name != NULL;
wouldn't is be safer to check if map is not NULL before deferencinf it?
Check added.
Is there a reason for incrementing base_iter at the end of
sdap_search_bases_next_base(), I think it would be more clear to increment it
in sdap_search_bases_done() immediately before calling
sdap_search_bases_next_base() again?
It depends on the point of view, I guess. This way the iteration logic
is all in one place which is better in my opinion.
0005:
+int sysdb_compare_usn(const char *a, const char *b)
+{
+ if (a == NULL) {
+ return -1;
+ }
+
+ if (b == NULL) {
+ return 1;
+ }
+
+ /* less digits means lower number */
this is only true as long as a and b do not start with 0. I would recommend to
add a
if (*a != '0' && *b != '0') {
I added a cycle to remove leading zeros.
+ if (strlen(a) < strlen(b)) {
+ return -1;
+ }
+
+ /* more digits means bigger number */
+ if (strlen(a) > strlen(b)) {
+ return 1;
+ }
+
+ /* now we can compare digits since alphabetical order is the same
+ * as numeric order */
+ return strcmp(a, b);
+}
+errno_t sysdb_get_highest_usn(TALLOC_CTX *mem_ctx,
+ struct sysdb_attrs **attrs,
+ size_t num_attrs,
+ char **_usn)
+{
+ const char *highest = NULL;
+ const char *current = NULL;
+ char *usn;
+ errno_t ret;
+ size_t i;
+
+ if (num_attrs == 0 || attrs == NULL) {
you can save a few lines since highest == NULL with:
goto done;
Done.
+#define ALL_FILTER "(" SYSDB_OBJECTCLASS "="
SYSDB_SUDO_CACHE_OC ")"
+
I would prefer a more specific name like e.g. SUDO_ALL_FILTER.
Done.
0008:
> @@ -516,6 +516,8 @@ static void sdap_sudo_refresh_done(struct tevent_req *subreq)
> tevent_req_error(req, ret);
> }
> return;
> + } else if (ret != EOK) {
> + tevent_req_error(req, ret);
Isn't a return missing here?
Yes, fixed.
0009:
+static bool check_dn(struct ldb_dn *dn,
+ const char *rdn_attr,
+ va_list in_ap)
+{
+ const struct ldb_val *ldbval;
+ const char *strval;
+ const char *ldbattr;
+ const char *attr;
+ const char *val;
+ va_list ap;
+ int num_comp;
+ int comp;
+
+ /* check RDN attribute */
+ ldbattr = ldb_dn_get_rdn_name(dn);
+ if (ldbattr == NULL || strcasecmp(ldbattr, rdn_attr) != 0) {
+ return false;
+ }
+
+ /* Check DN components. First we check if all attr=value pairs match input.
+ * Then we check that the next attribute is a domain component.
+ */
+
+ comp = 1;
+ num_comp = ldb_dn_get_comp_num(dn);
+
+ va_copy(ap, in_ap);
+ while ((attr = va_arg(ap, const char *)) != NULL) {
+ val = va_arg(ap, const char *);
+ if (val == NULL) {
+ return false;
+ }
+
+ if (comp > num_comp) {
+ return false;
+ }
+
+ ldbattr = ldb_dn_get_component_name(dn, comp);
+ if (ldbattr == NULL || strcasecmp(ldbattr, attr) != 0) {
+ return false;
+ }
+
+ ldbval = ldb_dn_get_component_val(dn, comp);
+ if (ldbval == NULL) {
+ return false;
+ }
+
+ strval = (const char *)ldbval->data;
+ if (strval == NULL || strncasecmp(strval, val, ldbval->length) != 0) {
+ return false;
+ }
+
+ comp++;
+ }
+ va_end(ap);
+
+ ldbattr = ldb_dn_get_component_name(dn, comp);
+ if (ldbattr == NULL || strcmp(ldbattr, "dc") != 0) {
If I see it correctly the "dc" is the only thing specific to IPA (and it would
work for AD as well). I would recommend to make the name of the domain
component a parameter of check_dn() and not put it in ipa/ipa_dn.c but
ldap/sdap_dn.c. This would avoid in the next patch to include a file from the
IPA provider in libsss_ldap_common_la_SOURCES.
Even though I don't like having it included in ldap_common.la, I think
it is a lesser evil. You can't use it in LDAP and AD, since you can't
expect any specific dn format there, or can you?
Also having "dc" value a parameter and making the function more general
would make the check and function call much more complicated since we
would have to check for a correct ending (i.e. dc=example,dc=com or
whatever is set) and not just for a presence of dc. But we can work on
that if you want, I just think it is unnecessary for the given usage.
+ return false;
+ }
+
+ return true;
+}
That's all for now, more will come later.
Thank you, patches are attached.