Red Hat Bugzilla – Bug 204517
Server needs to use new ber types
Last modified: 2015-12-07 11:42:29 EST
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
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
- 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
- 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
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.
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]
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.
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.