Bug 204517 - Server needs to use new ber types
Server needs to use new ber types
Status: CLOSED CURRENTRELEASE
Product: 389
Classification: Community
Component: Directory Server (Show other bugs)
1.0.2
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nathan Kinder
Viktor Ashirov
:
Depends On:
Blocks: 152373 fds103trackingbug 240316
  Show dependency treegraph
 
Reported: 2006-08-29 14:19 EDT by Nathan Kinder
Modified: 2015-12-07 11:42 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-12-07 11:42:29 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
CVS Diffs - First pass (35.00 KB, patch)
2006-08-29 14:31 EDT, Nathan Kinder
no flags Details | Diff
CVS Diffs - Second pass (62.97 KB, patch)
2006-08-30 14:54 EDT, Nathan Kinder
no flags Details | Diff
CVS Diffs - Third pass (66.27 KB, patch)
2006-08-30 17:58 EDT, Nathan Kinder
no flags Details | Diff
Revised Diffs (71.30 KB, patch)
2006-08-31 18:46 EDT, Nathan Kinder
no flags Details | Diff
CVS Commit Message (4.70 KB, text/plain)
2006-08-31 18:55 EDT, Nathan Kinder
no flags Details
CVS Diffs - Fourth pass (12.37 KB, patch)
2006-09-01 14:31 EDT, Nathan Kinder
no flags Details | Diff
Revised Diffs - Fourth pass (17.39 KB, patch)
2006-09-01 16:27 EDT, Nathan Kinder
no flags Details | Diff
CVS Commit Message (2.39 KB, text/plain)
2006-09-01 17:30 EDT, Nathan Kinder
no flags Details

  None (edit)
Description Nathan Kinder 2006-08-29 14:19:38 EDT
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.
Comment 1 Nathan Kinder 2006-08-29 14:31:45 EDT
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.
Comment 2 Rich Megginson 2006-08-29 16:52:47 EDT
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*/
Comment 3 Nathan Kinder 2006-08-30 14:54:41 EDT
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.
Comment 4 Noriko Hosoi 2006-08-30 15:12:02 EDT
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.)
Comment 5 Rich Megginson 2006-08-30 15:34:41 EDT
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.
Comment 6 Nathan Kinder 2006-08-30 17:58:50 EDT
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.
Comment 7 Nathan Kinder 2006-08-30 19:19:28 EDT
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.
Comment 8 Nathan Kinder 2006-08-31 18:46:49 EDT
Created attachment 135346 [details]
Revised Diffs

These diffs address the one issue I brought up in comment 7.
Comment 9 Nathan Kinder 2006-08-31 18:55:51 EDT
Created attachment 135347 [details]
CVS Commit Message

Checked fixes into ldapserver (HEAD).  Thanks to Rich and Noriko for reviews!
Comment 10 Nathan Kinder 2006-09-01 14:31:02 EDT
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().
Comment 11 Nathan Kinder 2006-09-01 16:27:19 EDT
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.
Comment 12 Rich Megginson 2006-09-01 16:34:41 EDT
Looks good.
Comment 13 Nathan Kinder 2006-09-01 17:30:26 EDT
Created attachment 135417 [details]
CVS Commit Message

Checked changes in from comment #11.  Thanks for the review Rich!
Comment 14 Rich Megginson 2006-10-09 17:25:26 EDT
Nathan, can this bug be marked as MODIFIED?
Comment 15 Nathan Kinder 2006-10-09 17:49:55 EDT
Yes, it can.  Marking as MODIFIED.
Comment 16 Rich Megginson 2008-01-02 16:34:18 EST
Verfied.  Server builds with new ldap c sdk code that uses the new ber types.

Note You need to log in before you can comment on or make changes to this bug.