Closed Bug 347933 Opened 18 years ago Closed 16 years ago

merging Sun and Mozilla.org liblber

Categories

(Directory :: LDAP C SDK, defect)

Product:
Directory
Component:
LDAP C SDK
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: neuroc0der, Assigned: mcs)

References

Details

Attachments

(5 files, 2 obsolete files)

resync with Bug 228704 changes
105.88 KB, patch
Details | Diff | Splinter Review
more lber porting
41.17 KB, text/plain
mcs
: review+
Details
windows build
10.46 KB, text/plain
Details
More lber porting
6.14 KB, patch
Details | Diff | Splinter Review
ctrls related changes
22.66 KB, patch
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6

this one is to track and review the merge of Sun and Mozilla.org code of liblber.


Reproducible: Always
the patch does a complete liblber merge. some parts of this patch are
outside liblber scope however they have to go with liblber changes
here. 

i did try my best to merge completely all the files involved even when
certain changes fall outside of lber context and those are mostly very
minor changes so they should not complicate or prolong reviews in any
way i reckon. there are two files that fall out of that completely
merged set: 

- mozilla/directory/c-sdk/ldap/libraries/libldap/request.c

complete merge in this case have to go thru separate review of request
/result code merge so my changes in this file are only in lber scope.

- mozilla/directory/c-sdk/ldap/libraries/libldap/saslbind.c

sun_merge_branch_20060523 code is used for merging so it contains what
Nathan checked in there re bugid 228704 but since its not over yet and
my changes there are minimal i can re-apply lber changes to the final
version once its available.

the high level overview of the differences between one code base and
the other is described here: http://wiki.mozilla.org/LDAPSunMerge#liblber

i want you guys to pay special attention to these two things below :

- introduction of proper ber types.

why do this?! there are two main reasons. firstly The C LDAP Application Program Interface draft [that got killed but in fact was a good writeup
to address compatibility issues among diff implementations] specifies
in its section 17.1. "BER Data Structures and Types" what types should
be used manipulating BER encoded ASN.1 values. i reckon its a good idea
to actually comply with that. secondly, OpenLDAP and Solaris native LDAP
implementations comply with the draft here and to stay compatible so
shall we. the other good reason tho its probably no concern here is that
implementing what draft says makes any future porting and same efforts
so much easier than if we are to stick with custom types.

- Mozilla bugid 221694, Sun bugid 4812593.

this is about 'restartable' ber_get_next*() issue/s. practically the 
same bug on both sides but addressed/fixed differently. i kept Sun 
version as it seemed more compact/better to me. if you have any test
cases just for that still lying somewhere under some heavy dust some
where i would appreciate you share them as i wanna make sure theres 
no regression being introduced here by bringing Sun ver of the fix.

other changes are mostly functionality enhancements and varies bugfixes
and optimizations to address performance issues but please review them.

on Sun side ber types change happened about 2 years ago so the code you
see here has been tested in several products, directly and indirectly.

and one last thing : done some successful smoketests of this patch on :

- Solaris  9 32b and 64b SPARC
- Solaris 10 32b and 64b x86-64
- RedHatES 4 32b and 64b x86-64
In general, the changes look good.  I'm shocked at how many longs have changed to int - I'm sure that was fun to debug all of those changes on 64 bit platforms (can you say bus error?).  These changes are going to wreak havoc with the directory server code . . .
The one function that looks really different is ber_get_next_buffer_ext.  Can you explain a bit more about why it was changed?

This is odd:
 	if ( *sos == NULL ) {
-		/*
-		 * No sequence or set to put... fatal error.
-		 */
+                /* No sequence or set to put... fatal error. */
 		return( -1 );
 	}
 
 	/*
 	 * If this is the toplevel sequence or set, we need to actually
 	 * write the stuff out.  Otherwise, it's already been put in
 	 * the appropriate buffer and will be written when the toplevel
 	 * one is written.  In this case all we need to do is update the
 	 * length and tag.
 	 */
 
+	if (*sos == NULL)
+		return (-1);

It checks for (*sos == NULL) twice?  Is this intentional (e.g. some sort of weird race condition) or a typo?

(In reply to comment #2)
> platforms (can you say bus error?).  These changes are going to wreak havoc
> with the directory server code . . .

it wasnt that bad for our DS. i would say it was pretty straightforward to
accomodate type changes there. most [ if not all ] of the stuff is getting
visible by throwing new lib in, building then reading compiler warnings on
type mismatches found. i'm not sure how much of a spilt in two DS srcbases 
there is [ Sun and RH trees i mean ] related. i think it should be roughly
the same for you guys.  

> The one function that looks really different is ber_get_next_buffer_ext.  Can
> you explain a bit more about why it was changed?

this is related to my original review request actually. the part about Moz 
bugid 221694, Sun bugid 4812593. there can be cases when pdu len encoded 
on multiple bytes and fragmentation split pdus in the middle of the len so
we cannot get it in one go and the idea is that we can reenter and get the
rest of it later when its available then glue the parts together. i'm not
how to reproduce sucha fragmentation because i think the only report we've
got is on Doze and that was a real pain to reproduce. thats the reason i'd
ask for any test cases on Mozilla side people might still have around. our
QA have their own testcase i think and i can ask for the details if needed
but since i kept Sun implementation [see original comment] i just want to 
make sure there is no possible indirect regression/s for Moz code. Noriko 
Hosoi wrote original Moz fix so if we can get his comments and review for 
this particular that would be real nice.

> This is odd:
> +       if (*sos == NULL)
> +               return (-1);
> 
> It checks for (*sos == NULL) twice?  Is this intentional (e.g. some sort of
> weird race condition) or a typo?

yep, its a dup. originally bugid 536911 [Netscape/iPlanet] / 166189 [Mozilla]
which as you well spotted got fixed two times and thats what i call quality :)
will go ahead and remove the second occurence.
Blocks: 339298
> most [ if not all ] of the stuff is getting
> visible by throwing new lib in, building then reading compiler warnings on
> type mismatches found.

The hardest part will be ber_printf/ber_scanf, or any other places that accept varargs or generic pointers.  But a global search/replace can take care of those.

> i just want to 
> make sure there is no possible indirect regression/s for Moz code. Noriko 
> Hosoi wrote original Moz fix so if we can get her comments and review for 
> this particular that would be real nice.

Ok.  cc'ing Noriko.
Thanks for including me.  The problem is found in the test "Reliab05 the multi-backend chaining test case. "  (I think the test case could be shared with Sun...)  The test case reported lots of Operations error:

Global error  1 (Operations error) occurs  1019 times

Debugging the problem, we found the length field fragmentation was happening and such a fragmentation case was not implemented then (2003-10-08).  So, I fixed it with Mark.
id=623793 (internal raidzilla bug#)
> connection_read_operation uses conn->c_private->c_buffer, which size is 512B. 
> Sometimes, a LDAP message is added from the offset 510.  That is, only the first
> 2 bytes are added.  Unfortunately, the message length could be longer than 128
> bytes, then the length is represented with multiple bytes.  In this case, the
> multiple byte length is cut in the middle.  Looking at ber_get_next_buffer_ext,
> it goes to "premature_exit" and returns the correct tag.  Bytes_Scanned is 2. 
> On the connection_read_operation size, it thinks the 2 bytes were consumed and
> read next 512 bytes.  The first byte for the length gets lost and wrong length
> is set to ber... 
>
> I think ber_get_next_buffer_ext should not return Bytes_Scanned == 2 since the
> first byte for the length was not really consumed...  And
> connection_read_operation should keep the last byte (or bytes in some cases) and
> concatinate the next read to the leftover byte(s).

I think Sun's fix also solved similar problems, but since there are many small differences, it may not fit in our server as is.  One obvious difference is Sun's implementation does not set errno in the error case.  Our server is relying on it like this:

    *tagp = ber_get_next_buffer_ext( buffer, buffer_size, lenp, ber,
            &bytes_scanned, conn->c_sb );
    if (LBER_DEFAULT == *tagp && 0 == bytes_scanned) {
        if (errno == EMSGSIZE) {
        ^^^^^^^^^^^^^^^^^^^^^^
            log_ber_too_big_error(conn, *lenp, 0);
            err = SLAPD_DISCONNECT_BER_TOO_BIG;
        } else {
            syserr = errno;
        }
Is there some way to get error codes from the Sun implementation?  I think the Sun DS also checks for too large BER sizes - how do they do it with this code?
(In reply to comment #6)
> Is there some way to get error codes from the Sun implementation?  I think the
> Sun DS also checks for too large BER sizes - how do they do it with this code?
> 

RHDS is depending upon this function ber_get_next_buffer_ext quite heavily.  There are some requirements from the server.  Basically, it's well described
in the comments:
/*
 * Returns the tag of the message or LBER_DEFAULT if an error occurs. There
 * are two cases where LBER_DEFAULT is returned:
 *
 * 1) There was not enough data in the buffer to complete the message; this
 *    is a "soft" error. In this case, *Bytes_Scanned is set to a positive
 *    number.
 *
 * 2) A "fatal" error occurs. In this case, *Bytes_Scanned is set to zero.
 *    To check for specific errors, the system error number (errno) must
 *    be consulted.  These errno values are explicitly set by this
 *    function; other errno values may be set by underlying OS functions:
 *
 *    EINVAL   - LBER_SOCKBUF_OPT_VALID_TAG option set but tag does not match.
 *    EMSGSIZE - length was not represented as <= sizeof(long) bytes or the
 *                  LBER_SOCKBUF_OPT_MAX_INCOMING_SIZE option was set and the
 *                  message is longer than the maximum. *len will be set in
 *                  the latter situation.
 */

When an real error occurs, we expect it's distinguished from the premature case.
To be more precise, when an error occurs, RHDS expects the error code set in errno and "*Bytes_Scanned = 0;".
The error cases we may see are
1)  errno = EINVAL
    if((sock->sb_options & LBER_SOCKBUF_OPT_VALID_TAG) && (tag != sock->sb_valid_tag))
            errno = EINVAL;
            *Bytes_Scanned = 0;
2) Given length is greater than what we can hold in an unsigned long.
2-1) if ( noctets > sizeof(unsigned long) )
            errno = EMSGSIZE;
            *Bytes_Scanned = 0;
2-2) if ( (sock != NULL)  &&
            ( sock->sb_options & LBER_SOCKBUF_OPT_MAX_INCOMING_SIZE )
                    && (*len > sock->sb_max_incoming) ) {
                        return( LBER_OVERFLOW );
        }
       Sun newly introduced LBER_OVERFLOW.
       RHDS expects errno = EMSGSIZE and *Bytes_Scanned = 0, here.
3) When realloc nslberi_ber_realloc fails:
            errno = ENOMEM;   // set in the system library
            *Bytes_Scanned = 0;

And it would be nice if you could use SAFEMEMCPY for memcpy.

We can change our code to use the return value rather than errno.  In general, it's not safe to depend on the value of errno in a multi-threaded environment, but it may be ok in this case if the OS does some tricks like putting errno in thread local storage.  I would rather not depend on errno.  Anton, other than this, how can we deal with the different error conditions returned by ber_get_next_buffer_ext()?
(In reply to comment #8)
> We can change our code to use the return value rather than errno.  In general,
> it's not safe to depend on the value of errno in a multi-threaded environment,
> but it may be ok in this case if the OS does some tricks like putting errno in
> thread local storage.  I would rather not depend on errno.  Anton, other than
> this, how can we deal with the different error conditions returned by
> ber_get_next_buffer_ext()?

But doesn't much of the POSIX (Unix) API depend on errno?  I am sure every platform that supports threads uses thread-local storage for errno.  I do not have time to analyze all of the changes to ber_get_next_buffer_ext() but if both sets of changes are solving the same problem I vote for keeping the existing Mozilla code (that seems easiest to me).
its maybe not a good thing to do but i will try to address all this in one
comment here.

- re EMSGSIZE/EINVAL RedHat DS depends on. i see no problem having them
  in there on top of proposed patch. this certainly doesnt clash with
  anything and my only concern that it is inconsistent with the rest of 
  liblber but since the changes were already brought in and products 
  depend on them now we gonna have to support that going forward.

- Noriko, which memcpy did i miss ? can you point me to it ? i thought
  i did replace all of them with SAFEMEMCPY.

- Noriko, from looking at new proposed code, apart from errno diffs
  is there anything else in there in terms of behavior you think is
  different ?

- re POSIX etc and errno being thread safe. from what i know its not
  recommended to use errno in multithreaded environment/s and return
  code/s ie something that lives on the stack [ per thread ] versus 
  global storage [ per proc ] is way safer to use even when errno is
  redefined as macro to support thread local storage but as i said 
  its no problem to use errno there on top of existing code.
Even though errno may be thread safe, I still don't like having to depend on a global variable to test the error status.  And I don't mind changing the server code to test for error conditions in a different manner - there are going to be big changes in the server code anyway for all of the other lber changes.  I would rather use a stack variable for the return/error code.  That being said, do we need to introduce another return code in order to cope with the errors that our server code currently deals with?  Or can we handle all of the error conditions we need to handle if we use the Sun implementation?
(In reply to comment #11)

i can modify the patch to use LBER_OVERFLOW for all cases you use EMSGSIZE for.
re EINVAL i'm not really sure i understand the need for it ie why LBER_ERROR/
DEFAULT with Bytes_Scanned == 0 cannot be used. i will redo the patch to keep
setting errno regardless so nobody suffers any regressions.

> would rather use a stack variable for the return/error code.  That being said,
> do we need to introduce another return code in order to cope with the errors
> that our server code currently deals with?  Or can we handle all of the error
> conditions we need to handle if we use the Sun implementation?
 


(In reply to comment #10)
> its maybe not a good thing to do but i will try to address all this in one
> comment here.
> 
> - Noriko, which memcpy did i miss ? can you point me to it ? i thought
>   i did replace all of them with SAFEMEMCPY.

You are right, Anton.  Sorry.  I thought one memcpy was left, but it was my misreading.
 
> - Noriko, from looking at new proposed code, apart from errno diffs
>   is there anything else in there in terms of behavior you think is
>   different ?

Other than the errno and "*Bytes_Scanned == 0", I think the two codes behave in the same way.  As long as the "soft" error (fragmentation) is distinguished from the real errors (EMSGSIZE, EINVAL, ENOMEM), we should be able to adjust to the new code...
(In reply to comment #11)
> Even though errno may be thread safe, I still don't like having to depend on a
> global variable to test the error status.  

Just to clarify a little more..., RHDS is not depending on errno to "test the error status".  Errno is used to determine whether logging the "ber too big error" and returning the appropriate error code: SLAPD_DISCONNECT_BER_TOO_BIG or not.  For the real error status checking, we use two values: if tag is LBER_DEFAULT AND bytes_scanned is 0 (LBER_DEFAULT == tag && 0 == bytes_scanned) or not.  So, we are not depending on the global variable that much.
Attached patch refreshed patch (obsolete) — — Splinter Review 
this new cut should address all review comments so far, please review again.
Attachment #232804 - Attachment is obsolete: true
Ok.  Looks good.  It also looks like there are several conflicts with the new SASL code.  I'd like to commit the new SASL code first before we commit these changes.
thanx. yeah sure i will wait til you check that sasl code in and then i shall resync my patch.
 
It looks good to me, too.  Thanks for your extra work to fulfill our requests!
only two files are affected: lber-int.h and saslbind.c, the rest is the same.
Attachment #234208 - Attachment is obsolete: true
Ok.  Looks good.
checked in on the merge branch :

Checking in mozilla/directory/c-sdk/ldap/include/lber.h;
/cvsroot/mozilla/directory/c-sdk/ldap/include/lber.h,v  <--  lber.h
new revision: 5.3.8.1; previous revision: 5.3
done
Checking in mozilla/directory/c-sdk/ldap/include/portable.h;
/cvsroot/mozilla/directory/c-sdk/ldap/include/portable.h,v  <--  portable.h
new revision: 5.14.8.1; previous revision: 5.14
done
Checking in mozilla/directory/c-sdk/ldap/libraries/liblber/decode.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/liblber/decode.c,v  <--  decode.c
new revision: 5.4.2.1; previous revision: 5.4
done
Checking in mozilla/directory/c-sdk/ldap/libraries/liblber/encode.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/liblber/encode.c,v  <--  encode.c
new revision: 5.3.8.1; previous revision: 5.3
done
Checking in mozilla/directory/c-sdk/ldap/libraries/liblber/io.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/liblber/io.c,v  <--  io.c
new revision: 5.8.8.1; previous revision: 5.8
done
Checking in mozilla/directory/c-sdk/ldap/libraries/liblber/lber-int.h;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/liblber/lber-int.h,v  <--  lber-int.h
new revision: 5.3.8.3; previous revision: 5.3.8.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/compat.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/compat.c,v  <--  compat.c
new revision: 5.2.8.1; previous revision: 5.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/control.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/control.c,v  <--  control.c
new revision: 5.2.8.1; previous revision: 5.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/error.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/error.c,v  <--  error.c
new revision: 5.4.8.1; previous revision: 5.4
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/extendop.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/extendop.c,v  <--  extendop.c
new revision: 5.3.8.1; previous revision: 5.3
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/getattr.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/getattr.c,v  <--  getattr.c
new revision: 5.2.8.1; previous revision: 5.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/psearch.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/psearch.c,v  <--  psearch.c
new revision: 5.2.8.1; previous revision: 5.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/result.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/result.c,v  <--  result.c
new revision: 5.6.8.1; previous revision: 5.6
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/saslbind.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/saslbind.c,v  <--  saslbind.c
new revision: 5.3.2.5; previous revision: 5.3.2.4
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/sortctrl.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/sortctrl.c,v  <--  sortctrl.c
new revision: 5.2.8.1; previous revision: 5.2
done
Attached file more lber porting — 
1) remove some casts for arguments to ber_scanf/ber_printf/ber_put*/ber_get* that are not needed anymore e.g. don't need to cast bv_val from long to int anymore (better still would be to use 'O')
2) found a couple of places that were using long * instead of int *.  I'm surprised these didn't cause problems on 64 bit platforms.
3) Use 0x%p instead of 0x%x or 0x%lx to print an address (e.g. LDAP *).  I guess 0x%lx would work on on 32bit and 64bit, but %p is portable and proper.
4) changed ldapmsg.lm_msgtype from int to ber_tag_t
5) changed lr_res_msgtype from int to ber_tag_t
Attachment #234953 - Flags: review?(mcs)
The changes attached in comment 22 look good.
Checking in mozilla/directory/c-sdk/ldap/clients/tools/common.c;
/cvsroot/mozilla/directory/c-sdk/ldap/clients/tools/common.c,v  <--  common.c
new revision: 5.17.8.4; previous revision: 5.17.8.3
done
Checking in mozilla/directory/c-sdk/ldap/clients/tools/ldapcompare.c;
/cvsroot/mozilla/directory/c-sdk/ldap/clients/tools/ldapcompare.c,v  <--  ldapcompare.c
new revision: 5.6.8.1; previous revision: 5.6
done
Checking in mozilla/directory/c-sdk/ldap/clients/tools/ldapmodify.c;
/cvsroot/mozilla/directory/c-sdk/ldap/clients/tools/ldapmodify.c,v  <--  ldapmodify.c
new revision: 5.5.8.1; previous revision: 5.5
done
Checking in mozilla/directory/c-sdk/ldap/libraries/liblber/dtest.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/liblber/dtest.c,v  <--  dtest.c
new revision: 5.2.8.1; previous revision: 5.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/liblber/io.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/liblber/io.c,v  <--  io.c
new revision: 5.8.8.2; previous revision: 5.8.8.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/cldap.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/cldap.c,v  <--  cldap.c
new revision: 5.2.8.1; previous revision: 5.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/compare.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/compare.c,v  <--  compare.c
new revision: 5.2.8.1; previous revision: 5.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/control.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/control.c,v  <--  control.c
new revision: 5.2.8.2; previous revision: 5.2.8.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/error.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/error.c,v  <--  error.c
new revision: 5.4.8.2; previous revision: 5.4.8.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/extendop.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/extendop.c,v  <--  extendop.c
new revision: 5.3.8.2; previous revision: 5.3.8.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/ldap-int.h;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/ldap-int.h,v  <--  ldap-int.h
new revision: 5.6.2.3; previous revision: 5.6.2.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/memcache.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/memcache.c,v  <--  memcache.c
new revision: 5.5.8.1; previous revision: 5.5
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/psearch.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/psearch.c,v  <--  psearch.c
new revision: 5.2.8.2; previous revision: 5.2.8.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/request.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/request.c,v  <--  request.c
new revision: 5.5.8.2; previous revision: 5.5.8.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/result.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/result.c,v  <--  result.c
new revision: 5.6.8.2; previous revision: 5.6.8.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/saslbind.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/saslbind.c,v  <--  saslbind.c
new revision: 5.3.2.6; previous revision: 5.3.2.5
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/sortctrl.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/sortctrl.c,v  <--  sortctrl.c
new revision: 5.2.8.2; previous revision: 5.2.8.1
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/vlistctrl.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/vlistctrl.c,v  <--  vlistctrl.c
new revision: 5.3.8.1; previous revision: 5.3
done
Attached file windows build — 
The windows build was broken:
1) Don't need to add -lsvrcore in svrcore.m4 - this is done in build.mk instead
2) Don't need cygwin_wrapper anymore - build is much faster without it too
3) Had to put stubs for the new public sasl functions
Attachment #235031 - Flags: review?(mcs)
The changes proposed in comment 25 look fine.
Checking in mozilla/directory/c-sdk/configure;
/cvsroot/mozilla/directory/c-sdk/configure,v  <--  configure
new revision: 5.51.2.5; previous revision: 5.51.2.4
done
Checking in mozilla/directory/c-sdk/configure.in;
/cvsroot/mozilla/directory/c-sdk/configure.in,v  <--  configure.in
new revision: 5.50.2.5; previous revision: 5.50.2.4
done
Checking in mozilla/directory/c-sdk/config/autoconf/svrcore.m4;
/cvsroot/mozilla/directory/c-sdk/config/autoconf/svrcore.m4,v  <--  svrcore.m4
new revision: 5.5.2.1; previous revision: 5.5
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/saslbind.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/saslbind.c,v  <--  saslbind.c
new revision: 5.3.2.7; previous revision: 5.3.2.6
done
Checking in mozilla/directory/c-sdk/ldap/libraries/msdos/winsock/nsldap32.def;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/msdos/winsock/nsldap32.def,v <--  nsldap32.def
new revision: 5.2.8.1; previous revision: 5.2
done
Comment on attachment 234953 [details]
more lber porting

Looks OK.  Should we change functions such as ldap_parse_sort_control() and ldap_parse_virtuallist_control() to use ber_int_t * instead of unsigned long * parameters?
Attachment #234953 - Flags: review?(mcs) → review+
Comment on attachment 235031 [details]
windows build

Are you sure the cygwin wrapper is not needed (seems like it was added for the Mozilla client builds, e.g., for TBird and its predecessors)?

Wouldn't it be better for the SASL stubs on Windows to return local error (or some other error code)?  Actually, I am not sure why stubs are needed.  Shouldn't those functions just be left out of the build?  Who is calling them?
theoretically we should, and i guess there are other places like that, tho
practically they are libldap public api and unlike liblber, which most
users wont even notice since there arent that many direct liblber users to
my knowledge, changing libldap public api can have pretty high visibility.

(In reply to comment #28)
> (From update of attachment 234953 [details] [edit])
> Looks OK.  Should we change functions such as ldap_parse_sort_control() and
> ldap_parse_virtuallist_control() to use ber_int_t * instead of unsigned long *
> parameters?
> 
The more I think about it, the more I am convinced that we will just get into trouble if we do not consistently use the ber typedefs.  I think functions like those I mentioned as well as ldap_msgtype() should use the typedefs.  I am sure there are other public functions in libldap that I missed as well.
The OpenLDAP functions do not use the ber types - for example,
LDAP_F( int )
ldap_parse_sort_control LDAP_P((
        LDAP           *ld,
        LDAPControl    **ctrlp,
        unsigned long  *result,
        char           **attribute ));

In fact, it just passes the result pointer directly into ber_scanf(ber, "e"), which seems like it would be a problem on 64 bit platforms since it casts the value to a ber_int_t*.  So I would prefer to keep it as it is, for maximal openldap compatability.
I see OpenLDAP using ber_int_t and ber_type_t in the ldap_ functions, e.g.,

  LDAP_F( int )
  ldap_parse_sortresponse_control LDAP_P((
	  LDAP *ld,
	  LDAPControl *ctrl,
	  ber_int_t *result,
	  char **attribute ));

You mentioned a function with a slightly different name, so maybe these changes are recent ones.  I am looking at the OpenLDAP trunk, i.e., http://www.openldap.org/devel/cvsweb.cgi/~checkout~/include/ldap.h?rev=1.320&hideattic=1&sortbydate=0

Just looked at their CVS logs, and found that these kinds of changes were made in January 2006:

http://www.openldap.org/devel/cvsweb.cgi/include/ldap.h.diff?r1=1.301&r2=1.302&hideattic=1&sortbydate=0&f=h
Ok.  I was just looking at what comes with FC5 (which is 2.3.1x).  Then, in that case, it would be better to do the work now to convert all of those functions to use the ber types instead of int, long, etc.
I found some areas where we were using ber_uint_t when we should be using ber_len_t.  These areas deal with the number of bytes to write, remaining, and max incoming.

Without these changes, there are some inconsistencies with the types used in the SDK.  It appears that OpenLDAP correctly uses ber_len_t for these options.
Re: comment #35: looks good
(In reply to comment #35 & #34)

looks good to me too. re ldap api changes to use ber types as openldap
does: Rich, are you working on this or i should go ahead and take it ?
Re: Comment #37: I think Nathan may have been working on it.  Nathan?  Anton, if you have the time, go ahead and fix it, otherwise I will.
I haven't worked on changing the public api yet.  I've only been working on the areas that my diffs address in comment 35.
Checked in changes proposed in comment 35.

Checking in ldap/libraries/liblber/io.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/liblber/io.c,v  <--  io.c
new revision: 5.8.8.3; previous revision: 5.8.8.2
done
Checking in ldap/libraries/liblber/lber-int.h;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/liblber/lber-int.h,v  <--  lber-int.h
new revision: 5.3.8.4; previous revision: 5.3.8.3
done
i think this should do it. plz have a good look and let 
me ken if i missed anything or got something wrong here
.
Re: Comment #41: Looks good.  Go ahead and commit.
Should ldap_msgtype() return a ber_tag_t?  I say yes although OpenLDAP has not yet made that change.

Should ldap_msgid() return a ber_int_t?  That would also imply that we should change the lm_msgid field inside the ldapmsg struct to a ber_int_t... and also change many integers in calls such as ldap_result().  I say "no" to this one.

for ldap_msgtype(), I agree with Mark.  Also about ldap_msgid(), but we are going to be changing the server side to use ber_int_t for the msgid.
(In reply to comment #42)
> Re: Comment #41: Looks good.  Go ahead and commit.
> 

ta, c'd in on sun_merge_branch_20060523.

Checking in mozilla/directory/c-sdk/ldap/clients/tools/ldapsearch.c;
/cvsroot/mozilla/directory/c-sdk/ldap/clients/tools/ldapsearch.c,v  <--  ldapsearch.c
new revision: 5.10.8.2; previous revision: 5.10.8.1
done
Checking in mozilla/directory/c-sdk/ldap/examples/psearch.c;
/cvsroot/mozilla/directory/c-sdk/ldap/examples/psearch.c,v  <--  psearch.c
new revision: 5.2.8.1; previous revision: 5.2
done
Checking in mozilla/directory/c-sdk/ldap/examples/srvrsort.c;
/cvsroot/mozilla/directory/c-sdk/ldap/examples/srvrsort.c,v  <--  srvrsort.c
new revision: 5.2.8.1; previous revision: 5.2
done
Checking in mozilla/directory/c-sdk/ldap/include/ldap-extension.h;
/cvsroot/mozilla/directory/c-sdk/ldap/include/ldap-extension.h,v  <--  ldap-extension.h
new revision: 5.4.8.4; previous revision: 5.4.8.3
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/psearch.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/psearch.c,v  <--  psearch.c
new revision: 5.2.8.3; previous revision: 5.2.8.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/sortctrl.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/sortctrl.c,v  <--  sortctrl.c
new revision: 5.2.8.3; previous revision: 5.2.8.2
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/test.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/test.c,v  <--  test.c
new revision: 5.4.8.1; previous revision: 5.4
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/vlistctrl.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/vlistctrl.c,v  <--  vlistctrl.c
new revision: 5.3.8.2; previous revision: 5.3.8.1
done


(In reply to comment #43)

both declared to return int in ldap-c-api-internet-draft document.

> Should ldap_msgtype() return a ber_tag_t?  I say yes although OpenLDAP has not
> yet made that change.
> 
> Should ldap_msgid() return a ber_int_t?  That would also imply that we should
> change the lm_msgid field inside the ldapmsg struct to a ber_int_t... and also
> change many integers in calls such as ldap_result().  I say "no" to this one.
> 


(In reply to comment #46)
> 
> both declared to return int in ldap-c-api-internet-draft document.

I would argue that that is just an oversight in the I-D but I really do not remember if the decision to return an int was a conscious one or not.

The problem I see is that if ber_tag_t needs to be something other than an int on certain platforms, ldap_msgtype() may not work correctly.  Do we know if this is a problem on any modern platform?  In the old days, we used longs for the ber tags so we could compile and run libldap correctly where sizeof(int) == 2.
On every platform we support (Win32 x86, Mac ppc/x86, Solaris sparc/x86, HP pa/ipf, linux x86) and most that we don't anymore (aix, tru64, sgi) sizeof(int) == 4.  I think only on DOS/Win16 is sizeof(int) == 2 - and probably more old, obscure platforms.
(In reply to comment #47)

Mark, i agree they need to be changed, ideally, however with OpenLDAP keeping
them as int right now and as noted by Rich we have it working anyway on every
platform we care about i would rather go for compatibility than 100% clean
implementation in this case. i think OpenLDAP kept them as int because of the
draft. another way to approach this is to ask OpenLDAP folks if they w/gonna
change their implementation and if so we can do the same, staying compatible.
OK, let's leave them as-is for now.  I filed bug 351554 to track this issue.
Ok.  These changes have been merged onto the trunk.  Can we close this bug now?
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment on attachment 235031 [details]
windows build

Clearing old review requests that appear to be no longer valid. If they are, please move the patches to open bugs and re-request review there.
Attachment #235031 - Flags: review?(mcs)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: