297131 - (ldapcontrols) basic support for LDAP v3 controls

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

A Thunderbird extension I'm working on wants a way to send an LDAP v3 control
with addressbook and autocomplete searches.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

This is going to involve adding some params to a bunch of methods in
nsILDAPOperation.  So all the LDAP callers will need to be touched, but the
changes are almost entirely generic search/replace changes.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v1

This is skeleton version of a patch, which has the basic interface changes I
propose fleshed out.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v2

Now with classinfo for some of the LDAP classes to quiet XPConnect shrieking.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v3

Now includes nsIAbLDAPDirectory.idl.

Comment 5

[Asa Dotzler [:asa]]

This is gonna need to make b4 if it's gonna make 1.1 so shifting the nomination.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v4

This patch is mostly code complete, and doesn't break LDAP autocomplete or
addressbook quick search.  Next step is to actually test to make sure that the
controls piece works.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v5

Various bugfixes, cleanups, and chock full of documentation comments.

Comment 8

[David :Bienvenu]

Comment on attachment 189801 [details] [diff] [review]
patch, v5

========
RCS file: directory/xpcom/base/public/

 The Initial Developer of the Original Code is 
 + * Netscape Communications Corporation. 
 + * Portions created by the Initial Developer are Copyright (C) 2005 
 + * the Initial Developer. All Rights Reserved. 

should be Oracle

It's confusing that these different constants have the same value and are
defined next to each other:

+  const unsigned long TAG_LBER_ERROR = 0xffffffff;
+  const unsigned long TAG_LBER_DEFAULT = 0xffffffff;

+  const unsigned long TAG_LBER_CONSTRUCTED = 0x20;
+  const unsigned long TAG_LBER_ENCODING_MASK = 0x20;

separating them or commenting as to what they specifically are so that it's OK
they're the same would be helpful.

 * @note Specifying a zero-length value is not currently supported at some 
 +   * date, setting this to an nsILDAPBERValue which has not had any of the
 +  
need a period and new sentence there, I think.

+nsLDAPBERElement::~nsLDAPBERElement()
+{
+  if (mElement) {
+    // anything inside here is not something that we own separately from
+    // this object, so free it
+    ber_free(mElement, 1);
+  }
+
+  return;

no need to return from the destructor here.


+  if (aValue) {
+    return NS_ERROR_NOT_IMPLEMENTED;
+  } else {
+    mElement = ber_alloc();

no need for the else. This could all just be:

if (aValue)
  retrn NS_ERROR_NOT_IMPLEMENTED;

mElement = ber_alloc();
return (mElement) ? NS_OK : NS_ERROR_OUT_OF_MEMORY;

+  int i = ber_flatten(mElement, &bv);
+
+  if (i < 0) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }

no need for the i var here, since we don't use it after this point.

r=bienvenu, with those nits.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v6

Fixed all of David's nits except the trailing return in the destructor, which I
think is better style, since it shows explicit intent.	Also fixed a gcc
warning. Carrying forward r+ from him.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 190495 [details] [diff] [review]
patch, v6

+    if (!length) {
+	 return NS_OK;
+    }

Need to initialize *aArray to something --- nsnull, at least

> NS_STATIC_CAST(nsILDAPControl *, control.get())

Why is that necessary?

I don't know anything about LDAP but the patch looks otherwise fine.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #10)
> (From update of attachment 190495 [details] [diff] [review] [edit])
> +    if (!length) {
> +	 return NS_OK;
> +    }
> 
> Need to initialize *aArray to something --- nsnull, at least

Fixed in the upcoming patch.

> > NS_STATIC_CAST(nsILDAPControl *, control.get())
> 
> Why is that necessary?

Otherwise gcc dies because control.get() returns an
|nsDerivedSafe<nsILDAPControl>*|.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v7

Fixed issue caught by review and a couple of comments.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Checked in.

Comment 14

[Scott MacGregor]

Looks like someone else already sr'ed this

Comment 15

[Wolfgang Rosenauer [:wolfiR]]

I think this broke --enable-ldap-experimental (at least in seamonkey):

nsLDAPChannel.cpp:199: error: `Abandon' undeclared (first use this function)

Comment 16

[Wolfgang Rosenauer [:wolfiR]]

build fix for ldap-experimental checked in with s+sr=dmose (given in IRC)