if the LDAP host is specified as multiple space-delimited hosts, ldap fails

NEW
Unassigned

Status

MailNews Core
LDAP Integration
14 years ago
6 years ago

People

(Reporter: Bienvenu, Unassigned)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

14 years ago
If the user/client specifies multiple space delimited ldap hosts, ldap lookups
fail. 4.x would use the first host, and then try the second if the dns lookup
failed on the first...
(Reporter)

Comment 1

14 years ago
Created attachment 134068 [details] [diff] [review]
proposed fix

this fix makes it so we'll at least connect to the first host. Down the road, I
need to fix this code to try multiple hosts. But for the near term, I need this
to work at least a little.

Updated

14 years ago
Blocks: 218713

Updated

14 years ago
Attachment #134068 - Flags: review+
(Reporter)

Comment 2

14 years ago
Comment on attachment 134068 [details] [diff] [review]
proposed fix

Dan, this is just temporary, but we need it for now.
Attachment #134068 - Flags: superreview?(dmose)

Comment 3

14 years ago
two more small comments, before you checkin:

1) "ldap c-dk" should be "ldap c-sdk"
2) -1 should be kNotFound

Comment 4

14 years ago
Comment on attachment 134068 [details] [diff] [review]
proposed fix

sr=dmose, but please adjust the comment a bit: "but now it fails completely"
will actually refer to pre-bugfix code, which is confusing for the reader.
Attachment #134068 - Flags: superreview?(dmose) → superreview+

Updated

14 years ago
OS: Windows 2000 → All
Hardware: PC → All
(Reporter)

Comment 5

14 years ago
Created attachment 134091 [details] [diff] [review]
fix addressing comments
Attachment #134068 - Attachment is obsolete: true
(Reporter)

Comment 6

14 years ago
Comment on attachment 134091 [details] [diff] [review]
fix addressing comments

carrying forward reviews
Attachment #134091 - Flags: superreview+
Attachment #134091 - Flags: review+
Attachment #134091 - Flags: approval1.6a?

Comment 7

14 years ago
Comment on attachment 134091 [details] [diff] [review]
fix addressing comments

a=asa (on behalf of drivers) for checkin to 1.6alpha
Attachment #134091 - Flags: approval1.6a? → approval1.6a+

Updated

14 years ago
Blocks: 222652
(Reporter)

Comment 8

14 years ago
fix to make it not fail checked in - I'll leave this open to make multiple hosts
actually work.
Status: NEW → ASSIGNED
(Reporter)

Comment 9

14 years ago
Here's the plan for making multiple hosts work:

Because we don't pass the host name directly into the ldap c-sdk ldap_init
function, but rather, do a dns lookup on the host and pass the resulting ip
address(es) into ldap_init(), we need to extend the code to look up multiple dns
hosts, and wait for the dns lookups to complete before calling ldap_init with
the ip addresses of all the hosts. We do all this because the ldap c-sdk does
blocking dns lookups, which blocks the UI. We believe the current trunk c-sdk
does async dns lookups, but we're using an older version which does not. The
long term plan should be to use the newer version of the c-sdk, but that seems
risky in the short term.

The draw back of doing multiple dns lookups is that we'll likely end up doing
more dns lookups than we need, if you assume the first host is up and running.
My guess is that the ldap c-sdk would only do one dns lookup in that case, but
we'll end up doing them all. But I think that's the price they're going to have
to pay until we can get to the newer version of the c-sdk.
Product: MailNews → Core

Updated

9 years ago
Assignee: bienvenu → nobody
Status: ASSIGNED → NEW
QA Contact: grylchan → ldap-integration

Updated

9 years ago
Assignee: nobody → bienvenu
(Assignee)

Updated

9 years ago
Product: Core → MailNews Core
(Reporter)

Updated

8 years ago
Assignee: bienvenu → nobody
You need to log in before you can comment on or make changes to this bug.