Bug 204517

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.