nsLDAPOperation::AddExt() issues incorrect warning : "AddExt can only add."

RESOLVED FIXED

Status

Directory
LDAP XPCOM SDK
--
minor
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Jeremy Lainé, Assigned: Jeremy Lainé)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

11 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.6) Gecko/20070723 Iceweasel/2.0.0.6 (Debian-2.0.0.6-1)
Build Identifier: 

When debugging is enabled, the nsLDAPOperation::AddExt() XPCOM wrapper around the ldap_add_ext() function issues a warning when it is called: "AddExt can only add.", even though the requested operation is indeed an "add".

The problem is that the check on the requested operation is incorrect. The following expression is tested:

 (operation | nsILDAPModification::MOD_ADD)

nsILDAPModification::MOD_ADD=0, so the test fails when operation is equal to MOD_ADD.



Reproducible: Always
(Assignee)

Comment 1

11 years ago
Created attachment 279359 [details] [diff] [review]
Fix the test on the LDAP operation
(Assignee)

Updated

11 years ago
Attachment #279359 - Flags: superreview?(bienvenu)
Attachment #279359 - Flags: review?(bienvenu)
Assignee: dmose → jeremy.laine
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

11 years ago
I'd be curious to hear from Standard8/dmose but it looks to me like the operations are exclusive, i.e., they're not flags where you could have an operation that's both an EDIT and an ADD. If they are flags, than ADD should be a 1, not 0, and the others should be 2 and 4. If they're not flags, the check in the assertion should simply be operation == nsILDAPModification::MOD_ADD
(Assignee)

Comment 3

11 years ago
The operations are indeed exclusive.

One note of caution though : you can't just test operation == nsILDAPModification::MOD_ADD, because "operation" can also carry the nsILDAPModification::MOD_BVALUES flag (0x80).

Another possible fix is:
1/ in nsILDAPModification.idl define nsILDAPModification::MOD_OPERATION = 0x03 
2/ in the assertion test (operation & nsILDAPModification::MOD_OPERATION == nsILDAPModification::MOD_ADD) 

Comment 4

11 years ago
ah, true, then what we'd do is turn off the high bit(s) before testing for equality, e.g., (operation & 0x7f) == nsILDAPModification::MOD_OPERATION
(Assignee)

Comment 5

11 years ago
Created attachment 279702 [details] [diff] [review]
 Fix the test on the LDAP operation v2

I did some more looking around and found the following:

a/ we must not change the values of nsILDAPModification::MOD_(ADD|DELETE|REPLACE|BVALUES), these values are defined so as to match LDAP_MOD_(ADD|DELETE|REPLACE|BVALUES)

b/ LDAP_MOD_BVALUES is the only flag that can be set, and so throughout the directory/* code, the actual operation is retrieved by doing (operation & ~LDAP_MOD_BVALUES)

c/ in terms of style, throughout nsLDAPModification.cpp, the LDAP_MOD_x form is used and not the long nsILDAPModification::MOD_x

I am attaching a new patch that reflects these findings.
Attachment #279359 - Attachment is obsolete: true
Attachment #279702 - Flags: superreview?(bienvenu)
Attachment #279702 - Flags: review?(bienvenu)
Attachment #279359 - Flags: superreview?(bienvenu)
Attachment #279359 - Flags: review?(bienvenu)

Comment 6

11 years ago
Comment on attachment 279702 [details] [diff] [review]
 Fix the test on the LDAP operation v2

good one, thx!
Attachment #279702 - Flags: superreview?(bienvenu)
Attachment #279702 - Flags: superreview+
Attachment #279702 - Flags: review?(bienvenu)
Attachment #279702 - Flags: review+
I've just checked this in on behalf of Jeremy.

Thanks for the patch :-)
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.