In order to use the latest Mozilla LDAP C SDK in the directory server, we need to start using the new ber types (ber_tag_t, ber_len_t, ber_int_t, etc.). We should do a check for any compiler warnings when using the new LDAP SDK as a first pass to find areas in the code where we need to use the new types. After we do that, we will have to do a manual scan to check the usage of the ber_printf(), ber_scanf() and ber_get_*() functions.
Created attachment 135159 [details] CVS Diffs - First pass These diffs are from scanning the compiler output for warnings related to the new ber types. This mainly caught places where we need to use ber_tag_t and ber_len_t. I also fixed the usage of ber_scanf() in nearby code.
hmm - looks like op->o_msgid needs to be ber_int_t. I don't think we need to cast mod_op to unsigned long: int mod_op; /* kind of mod + form of values*/
Created attachment 135235 [details] CVS Diffs - Second pass This pass fixes the issues that Rich raised in his review. I also scanned the code to check our usage of the ber_get_*() and ber_scanf() functions.
Looks good. (I reviewed vlv.c and connection.c more in depth: The former has index width change on the 64-bit machine, and connection.c is related to https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=204566.)
There are a couple of variable names that begin with long_ - long_id and long_mod_op. These variables are not of type long anymore and the name could be confusing. In the first case, I don't think we need long_id anymore, we can just use id. In the second case, I think we can just pass in &mod->mod_op and eliminate the temporary variable. Whoops, also long_changetypesp The calls to slapi_log_* - we should no longer print the tag as a %lu, just a %u, and then we can remove the cast to (unsigned long). It's generally better not to cast unless necessary - let the compiler find errors for us. Other than these minor things, looks good.
Created attachment 135247 [details] CVS Diffs - Third pass This new set of diffs addresses the issues raised in the most recent reviews. Here's what was changed from the last diffs: - In abandon.c, removed long_id and just used id instead, but redefined as a ber_int_t. - In bind.c, removed long_method and just used method instead, but redefined as a ber_tag_t. Also got rid of ber_version and just use version directly, but redefined as a ber_int_t. - In modify.c, I changed the name of long_mod_op to just plain mod_op. I looked into just passing &mod->mod_op into ber_scanf, but the mod_op member of the LDAPMod struct is a plain int, not a ber_int_t. - In psearch.c, changed ps_parse_control_value() and ps_add() to take a ber_int_t argument, which eliminates long_changetypesp. Changed all locations that call these functions as well. - In search.c, removed the long_* variables, changed the regular variables that weren't prefixed by "long_" to ber_int_t types, and passed them into ber_scanf() directly. - In vlv.c, removed the long_* variables and passed the elements of the vlv structure into ber_scanf() directly. I also noticed some error in the vlv_parse_request_control() function where we were using the wrong return type for ber_scanf(), and not even checking for errors being returned in one case. There was also a missing "else" in this function that would have caused a crash if ber_scanf() returned an error. - In result.c, changed the printf format from %lu to %u for printing tags. Also eliminated the casting of the tags to unsigned long. - In repl5_total.c, removed an unneeded cast in a call to slapi_print_error() where we are printing a ber_int_t.
There is one minor correction that needs to be made to my diffs from Comment 6. I had mentioned that there was a missing "else" in vlv.c taht would have caused a crash. This is not the case. The "else" is truly not needed as the second code block in the "LDAP_TAG_VLV_BY_VALUE" case is merely there to print a debug message. This debug message is harmless, even if the previous call to ber_scanf() returns LBER_ERROR. The reason this block of code is harmless is that we initizlize vlvp->value.bv_len and vlvp->value.bv_val to 0 and NULL respectively at the top of the vlv_parse_request_control. I will attach a new set of diffs that have this change, but the CVS server is down at the moment, so I can't generate the new diffs. All of the other changes in Comment 6 still apply.
Created attachment 135346 [details] Revised Diffs These diffs address the one issue I brought up in comment 7.
Created attachment 135347 [details] CVS Commit Message Checked fixes into ldapserver (HEAD). Thanks to Rich and Noriko for reviews!
Created attachment 135403 [details] CVS Diffs - Fourth pass I did another pass on the code, this time checking our usage of ber_printf(). These diffs address the types that we pass into ber_printf().
Created attachment 135410 [details] Revised Diffs - Fourth pass After looking at Anton's fixes in the Mozilla LDAP C SDK bug 347933, I realized that I need to make some adjustments to the ber types being used for the vlv control structs. We should be using ber_int_t instead of ber_uint_t as my previous fix did. Both the Mozilla LDAP C SDK and the OpenLDAP libraries use ber_int_t for the vlv control fields.
Looks good.
Created attachment 135417 [details] CVS Commit Message Checked changes in from comment #11. Thanks for the review Rich!
Nathan, can this bug be marked as MODIFIED?
Yes, it can. Marking as MODIFIED.
Verfied. Server builds with new ldap c sdk code that uses the new ber types.