351020 - ldap_add_result_entry library entry is LOCL in libldap.so

Status: RESOLVED FIXED

(Directory :: LDAP C SDK, enhancement)

Description

[jiri.sasek]

User-Agent:       Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.7) Gecko/20060120
Build Identifier: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.7) Gecko/20060120

ldap_add_result_entry can not be linked/called from the user application code. Also the:

nm libraries/libldap/SunOS5.11_i86pc_OPT.OBJ/libldap60.so | less
...is showing:
[671]   |     22832|    1177|FUNC |GLOB |0    |11     |ldap_add_ext
[377]   |     24064|     122|FUNC |GLOB |0    |11     |ldap_add_ext_s
[60]    |     93152|      20|FUNC |LOCL |2    |11     |ldap_add_result_entry
[559]   |     24016|      42|FUNC |GLOB |0    |11     |ldap_add_s
[418]   |     41136|      35|FUNC |GLOB |0    |11     |ldap_ber_free
[398]   |     24592|      87|FUNC |GLOB |0    |11     |ldap_bind

...so the ldap_add_result_entry is not exported by the libldap library

Reproducible: Always

Steps to Reproduce:
1. nm libldap.so
2. look on ldap_add_result_entry
Actual Results:  
[60]    |     93152|      20|FUNC |LOCL |2    |11     |ldap_add_result_entry

Expected Results:  
[60]    |     93152|      20|FUNC |GLOB |2    |11     |ldap_add_result_entry

ldap_add_result_entry have to be named in the .spec file

ldap_add_result_entry is neede by Samba when it is built with the Active Directory support

Comment 1

[Mark Smith [:mcs]]

In OpenLDAP these two functions are public:
  ldap_add_result_entry()
  ldap_delete_result_entry()
(with prototypes in ldap.h and presumably with appropriate exports in shared object/DLL definition files).

I have no problem exposing these.  Rich and Anton, what do you think?

Comment 2

[Anton Bobrov]

Mark, you are too fast [good thing]! :) below is the comment i been trying 
to submit before getting into mid-air collision. additionally i wanna note 
that this is for our Samba folks so they can have less broblems building 
with our ldap libs.

Original comment:
=================

Mark, just to give you some more info here. OpenLDAP exposed these two calls

LDAP_F( LDAPMessage * ) 
ldap_delete_result_entry LDAP_P((
        LDAPMessage **list,
        LDAPMessage *e ));
 
LDAP_F( void )
ldap_add_result_entry LDAP_P((
        LDAPMessage **list,
        LDAPMessage *e ));

in their ldap.h while ago, while we, probably for historical reasons, kept 
them private in our ldap-int.h.

i dont think there is any risk making them public so if you dont have any
objections i'm gonna prep a patch for review to make them public. i wanna
apply changes to sun_merge branch so that i can backport the same changes
to the private Sun branch inhouse. dunno if there is a need to have them
on the main branch right now tho.

Comment 3

[Rich Megginson]

Yes, let's make them public.  We are testing the last batch of sun merge branch code now.  We should pick up a new drop soon - it would be good to get these in.  Once we test them, we'll put them on the trunk.

Comment 4

[Anton Bobrov]

Attachment: patch rev 0

Comment 5

[Mark Smith [:mcs]]

Comment on attachment 236692 [details] [diff] [review]
patch rev 0


>+580 ldap_delete_result_entry
>+581 ldap_add_result_entry

What process are we using to assign ordinal numbers?  I assume it is hopeless to match those used by OpenLDAP, assuming they still assign numbers?


>Index: mozilla/directory/c-sdk/ldap/libraries/libldap/reslist.c
> ...
> void
> ldap_add_result_entry( LDAPMessage **list, LDAPMessage *e )
> {
>+    if ( list != NULL || e != NULL ) {
> 	e->lm_chain = *list;
> 	*list = e;
>+    }
> }

The check should be:
  if ( list != NULL && e != NULL ) {
(replace || with &&).

Comment 6

[Anton Bobrov]

(In reply to comment #5)

> >+580 ldap_delete_result_entry
> >+581 ldap_add_result_entry
> 
> What process are we using to assign ordinal numbers?  I assume it is hopeless
> to match those used by OpenLDAP, assuming they still assign numbers?

first off i dunno if OpenLDAP is actually using any mapfiles or at least i 
dont see any at the first glance.

regarding the numbers themselves i dont think they actually mean anything
[ or is it something Doze specific ? ] the only reason i put numbers there 
is because i'm not familiar with gen export script and thought it might 
depend on having them as first field. anyways if they are still used eg on
Doze i dont see any precise pattern or rule that could have been used to 
assign those numbers apart from the likes of pick some unused number which
is a bit away from the last used number which is what i have done as well.

> The check should be:
>   if ( list != NULL && e != NULL ) {
> (replace || with &&).

yep, nasty cut & paste on my part, sorry about that and thanx for spot on. 

Comment 7

[Anton Bobrov]

Attachment: patch rev 1

fixed "if" statement as suggested.

Mark, Rich, any further updates/opinions/newintel on what you wanna do
regarding mapfile numbers here ?

Comment 8

[Rich Megginson]

I think the new diff looks good.  Do we really care about what the Win32 function ordinal numbers are?

Comment 9

[Mark Smith [:mcs]]

(In reply to comment #8)
> I think the new diff looks good.  Do we really care about what the Win32
> function ordinal numbers are?

Probably not.  My only concern is that if they are used by the dynamic loader on Windows (and I am not sure if they still are or not but I think so), then it would be nice to use the same numbers as other LDAP C API implementations... but we have other incompatibilities anyway, so let's plow ahead.

Comment 10

[Anton Bobrov]

yeah. the thing is, the only situation i can imagine were having the same
numbers would be important is when one wants to drop mozilla libldap bin
in place of openldap libldap bin, without rebuilding. i dont see anything
similar in openldap tree today and i'm not aware of any other open source
implementation/s were we can look it up to stay on common ground. due to
other various diffs in implementations i reckon its unrealistic to expect
sucha drop replacement approach in the near future and if we ever reach
the point were that would be possible and *if* this doze optimization get
in our way we can revisit it then. i will go ahead and ci on sun_merge.  

(In reply to comment #9)
> Probably not.  My only concern is that if they are used by the dynamic loader
> on Windows (and I am not sure if they still are or not but I think so), then it
> would be nice to use the same numbers as other LDAP C API implementations...
> but we have other incompatibilities anyway, so let's plow ahead.

Comment 11

[Anton Bobrov]

Mark, Rich, once again, thanx for review!

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.5; previous revision: 5.4.8.4
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap.ex;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap.ex,v  <--  libldap.ex
new revision: 5.2.8.3; previous revision: 5.2.8.2
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.4; previous revision: 5.6.2.3
done
Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/reslist.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/reslist.c,v  <--  reslist.c
new revision: 5.2.8.1; previous revision: 5.2
done

Comment 12

[Anton Bobrov]

Attachment: consistent func declaration

i had Doze build failed last night and this is to make VC6 happy.

Comment 13

[Anton Bobrov]

Checking in mozilla/directory/c-sdk/ldap/libraries/libldap/reslist.c;
/cvsroot/mozilla/directory/c-sdk/ldap/libraries/libldap/reslist.c,v  <--  reslist.c
new revision: 5.2.8.2; previous revision: 5.2.8.1
done

Comment 14

[Rich Megginson]

This has been merged on to the trunk.  We can resolve this as FIXED.

Comment 15

[Mark Smith [:mcs]]

Reassigned to Anton (for tracking purposes) and marked fixed.