attachment
0001-Ticket-48805-Misleading-indent-and-Uninitialised-str.patch

No logic changes, mainly whitespace. I have run the test suite over this to ensure no accidental regressions were added.

Nice, ack!

Supplementary patch to fix many sign comparison warnings.
0001-Ticket-48805-Sign-comparison-checks.patch

Again, ran tests to ensure no regressions.

print_access_control_summary( char source, int ret_val, char clientDn,
{{{
662 662 struct codebook {
663 int code;
663 uint code;
664 664 char *text;
}}}
It's better to use the same type instead of the "default" type?
line 663 aclReasonCode_t code;

sha_pw_cmp (const char userpwd, const char dbpwd, unsigned int shaLen )
{{{
53 int hash_len; / must be a signed valued -- see below /
53 PRUint32 hash_len; / must be a signed valued -- see below /
}}}
But PRUint32 is unsigned... And it looks to me that pwdstorage_base64_decode_len does not return any negative value... so, we could remove the comment / must be a signed valued -- see below /?

Note: typedef unsigned int PRUint32;

static int printable_validate(
{{{
1330 1330 int rc = 0; / assume the value is valid /
1331 int i = 0;
1331 uint i = 0;
}}}
indentation???

do_add( Slapi_PBlock pb )
{{{
57 / ber_len_t is a typedef uint. Can't be -1 /
57 58 ber_len_t len = -1;
}}}
Instead of -1, why don't you use LBER_ERROR?
{{{
519 / Curiously, LBER_ERROR is uint, but set to -1 (0xffffffffU), even though ber_printf is int ... */
}}}
Casting to ber_tag_t makes -1 an unsigned int.
{{{

define LBER_ERROR ((ber_tag_t) -1)

}}}
So, (ber_tag_t) is doing the job here...

The same change could be applied to get_ldapmessage_controls_ext, get_filter_list( Connection conn, BerElement ber.

Please do not set 0 in get_filter_list. Set LBER_ERROR, instead.
{{{
387 ber_len_t len = -1;
387 / len, ber_len_t is uint. Can not be -1 /
388 ber_len_t len = 0;
}}}

{{{
int import_fifo_validate_capacity_or_expand(ImportJob job, int entrysize) {
}}}
Looking at the callers, size_t is being passed. Better change the function itself to this?
{{{
int import_fifo_validate_capacity_or_expand(ImportJob job, size_t entrysize)
}}}

static int ldbm_config_db_cache_set(void arg, void value, char *errorbuf, int
{{{
1059 if ((int)val > li->li_dblayer_private->dblayer_cache_config) {
1060 delta = (int)val - li->li_dblayer_private->dblayer_cache_config;
}}}
Instead, we should declare val as "int"?
{{{
1045 size_t val = (size_t) ((uintptr_t)value);
}}}
Plus, this "val" is not used in this debug print?
{{{
1063 LDAPDebug1Arg(LDAP_DEBUG_ANY,"Error: db cachesize value is too large.\n", val);
}}}

perfctrs_as_entry( Slapi_Entry e, perfctrs_private priv, DB_ENV *db_env )
{{{
291 size_t i;
}}}
I'm curious.... Why this type is size_t and the similar variable in moddn_rename_children is "uint"? What's the difference? I see many changes to "size_t i;"...

chown_dir_files
{{{
205 * If both is set, this would set gid_t to -1 then the chown would fail
206 * because the check in slapd_chown_if_not_owner requires both uid && gid to pass.
}}}
??? Sorry, I still don't understand...

slapd_chown_if_not_owner(const char filename, uid_t uid, gid_t gid)
{{{
1159 /
1160 * The getpwnam man page shows that either the caller will get
1161 * a NULL pw struct, or it will be populated. So pw->pw_uid will
1162 * have a sane value. Given that uid_t is a ushort, checking
1163 * for -1 is pointless, we actually break fchown assumptions doing
1164 * it. Read the fchown man page.
1165 * Same for gid_t
1166 * if (((uid != -1) && (uid != statbuf.st_uid)) ||
1167 * ((gid != -1) && (gid != statbuf.st_gid)))
1168 */
1169 if ((uid != statbuf.st_uid) || (gid != statbuf.st_gid))
}}}
??? Isn't it a spec of slapd_chown_if_not_owner? I guess the author wanted to add an ability to change just uid or just gid without getting the current value of not changing id? With your change, the caller of slapd_chown_if_not_owner needs to get the current uid/gid to make it intact?

notes2str( unsigned int notes, char buf, size_t buflen )
{{{
1897 / SLAPI_NOTEMAP_COUNT uses sizeof, size_t is unsigned. Was int */
1898 uint i;
}}}
indentation?

Applied all these recommendations, they are good points ....

---

perfctrs_as_entry( Slapi_Entry e, perfctrs_private priv, DB_ENV *db_env )
{{{
291 size_t i;
}}}
I'm curious.... Why this type is size_t and the similar variable in moddn_rename_children is "uint"? What's the difference? I see many changes to "size_t i;"...

Okay, the for statement here is:

{{{
for ( i = 0; i < SLAPI_LDBM_PERFCTR_AT_MAP_COUNT; ++i ) {
}}}

Here is the definition of SLAPI_LDBM_PERFCTR_AT_MAP_COUNT

{{{

define SLAPI_LDBM_PERFCTR_AT_MAP_COUNT \

(sizeof(perfctr_at_map) / sizeof(SlapiLDBMPerfctrATMap))

}}}

sizeof returns a size_t. So we make i a size_t so that we don't have a sign compare warning.

Same is true of many other cases for strlen, sizeof, etc. That's why there were lots of these changed.

---

??? Isn't it a spec of slapd_chown_if_not_owner? I guess the author wanted to add an ability to change just uid or just gid without getting the current value of not changing id? With your change, the caller of slapd_chown_if_not_owner needs to get the current uid/gid to make it intact?

I sat here thinking about this for a while, and rethinking I can see the original intent of the author now. I'm going to undo this "fix". I see what is happening here now. Thanks for the review.

The point of the original if was that if the -1 was there, it would trigger (uid != statbuf.st_uid), even if statbuf.st_uid was already correct. This statement is meant to avoid un-needed chown's, but I was thinking about it incorrectly. And the way it's structured, it will pass through the -1, to ignore correctly if the other meets the condition.

I'm rebuilding and testing now.

Supplementary patch to fix many sign comparison warnings, updated with Noriko's feedback.
0001-Ticket-48805-Sign-comparison-checks.2.patch

Thank you so much for cleaning up the code, William!!

There is one more place you'd better use LBER_ERROR for -1 and remove the comment (well, the comment is correct, tho. :). The rest looks good. You have my ack. Once control.c is fixed, please push to master which is for 1.3.6 and newer now (Thanks to, Mark)!
{{{
ldap/servers/slapd/control.c
diff --git a/ldap/servers/slapd/control.c b/ldap/servers/slapd/control.c
179 / ber_len_t is uint, cannot be -1 /
179 180 ber_len_t len = -1;
}}}

Fixing a compiler warning:
19e75b9..590799f master -> master
commit 590799f

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1865

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.