NSPR should provide getaddrinfo

RESOLVED FIXED in 4.5

Status

defect
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: darin.moz, Assigned: darin.moz)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

16 years ago
NSPR should provide a function like getaddrinfo (RFC 2553), which is now
available on many operating systems.  In particular this function is needed in
order to support IPv6 under WinXP.
(Assignee)

Comment 1

16 years ago
Posted patch v1 patch (obsolete) — Splinter Review
This is based on an API that wtc and I came up with a couple weeks ago.
(Assignee)

Comment 2

16 years ago
I have tested this patch under WinXP, Linux-RH9, and MacOSX 10.2.  I plan to
test it out on other versions of windows shortly.  Any help making sure it does
the right thing on other platforms would be appreciated.

Actually, I'm pretty sure we don't have |struct addrinfo| on all UNIX platforms,
but the patch assumes that we do.  That'll have to change.
Status: NEW → ASSIGNED
(Assignee)

Updated

16 years ago
Attachment #126959 - Flags: review?(wtc)
(Assignee)

Comment 3

16 years ago
Posted patch v1.1 patch (obsolete) — Splinter Review
slightly revised patch:

 o  PR_GetAddrInfoByName now includes |af| and |flags| parameters.
 o  wrapped calls to dlopen/dlsym/dlclose with #ifdef USE_DLFCN.
 o  set correct error code when getaddrinfo fails.
Attachment #126959 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #127501 - Flags: review?(wtc)
(Assignee)

Updated

16 years ago
Attachment #126959 - Flags: review?(wtc)
(Assignee)

Updated

16 years ago
Attachment #127501 - Attachment is obsolete: true
Attachment #127501 - Flags: review?(wtc)
(Assignee)

Comment 4

16 years ago
Posted patch v1.2 patch (obsolete) — Splinter Review
this patch uses PR_FindFunctionSymbolAndLibrary instead of platform specific
methods.  it also fixes some other potential compilation problems on other
platforms.  i've compiled this under WinXP (MSVC6), RH9-Linux (GLIBC 2.3),
MacOSX 10.2.
(Assignee)

Comment 5

16 years ago
Posted patch v1.3 patch (obsolete) — Splinter Review
left out one thing from last patch.
Attachment #127557 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #127558 - Flags: review?(wtc)
(Assignee)

Comment 6

16 years ago
cc'ing some folks involved with mozilla ports.  if you guys could compile the
v1.3 patch for me, and let me know if it breaks and/or let me know if you see
anything in it that needs to be modified for your platform, it'd be greatly
appreciated.  also, if you know of any other ports maintainers that should be
cc'd, please add them.  thanks!

Comment 7

16 years ago
For OpenVMS I need the ability to specify at run time the name of the symbol
that we try to dynamically look up, in the same way that we already do for
GETIPNODEBYXXXX and FREEHOSTENT. See
http://lxr.mozilla.org/nspr/source/nsprpub/pr/src/io/pripv6.c#295 for details.
(Assignee)

Comment 8

16 years ago
colin: so, OpenVMS does indeed provide getaddrinfo then?

Comment 9

16 years ago
> colin: so, OpenVMS does indeed provide getaddrinfo then?

In more recent versions of TCP/IP Services for OpenVMS, which have support for
IPv6, yes. But not everyone who is running TCP/IP Services for OpenVMS (UCX) is
running a recent version.

And UCX is only one of three commonly used IP stacks on OpenVMS. I don't think
the other two are as far along in their IPv6 support.
(Assignee)

Comment 10

16 years ago
ok, thanks colin... i'll modify the patch as you suggested.
(Assignee)

Comment 11

16 years ago
Posted patch v1.4 patch (obsolete) — Splinter Review
revised patch:

1- added getenv calls for VMS
2- added AI_ADDRCONFIG per wtc (following apache's usage of getaddrinfo)
Attachment #127558 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #127558 - Flags: review?(wtc)
(Assignee)

Comment 12

16 years ago
Comment on attachment 127743 [details] [diff] [review]
v1.4 patch

wtc: nevermind the "RFC 2553" mentioned in the .h file... i've changed that to
RFC 3493 in my source tree :)
Attachment #127743 - Flags: review?(wtc)

Comment 13

16 years ago
For BeOS:  I will verify this the week after next.  Sorry I can't do it sooner (was in thehostpital, now going on vacation)  I will let you know when I get back
Builds on OS/2.  Other than trying to build this with the nsDnsService rewrite,
is there a way to test this new function?
(Assignee)

Comment 15

16 years ago
Javier: no, there isn't any NSPR test for these functions (yet).  if you want to
easily apply the full DNS rewrite patch, you can do so by downloading the entire
patch (including the changes to NSPR) from here:

  http://friedfish.homeip.net/builds/dns/dns-20030718.diff

NOTE: this is a dyndns hostname, so you might have trouble loading this page if
your firewall blocks internal access to dyndns hosts (as is the case here at AOL).
Darin:  Thanks for the patch. I found that I wasn't exporting the new functions
out of NSPR. I need to add this to nsprpub/pr/src/nspr.def:

;+NSPR_4.5 {
;+	global:
		PR_GetAddrInfoByName;
		PR_FreeAddrInfo;
		PR_EnumerateAddrInfo;
		PR_GetCanonNameFromAddrInfo;
;+} NSPR_4.4;

However, I just made those NSPR numbers up.  WTC, is the correct thing to do? 
Should I just put everything in the NSPR_4.4 block?

Comment 17

16 years ago
These new functions should be in the NSPR_4.5 block.
(NSPR 4.4, the version of NSPR in Mozilla 1.4, has
been released.)

Comment 18

16 years ago
Posted patch OpenVMS changes needed (obsolete) — Splinter Review
Wan-Teh, we also need the same nspr.def changes in the OpenVMS equivalent.
Patch is attached. I also noticed that PR_GetPathSeparator snuck into nspr.def
without being added to nspr_symvec.opt, so I've added that too.
Posted patch nspr.def changes (obsolete) — Splinter Review
Patch for the nspr.def changes.  With this, it builds and works on OS/2 (tested
with full nsDnsService rewrite patch).
(Assignee)

Comment 20

16 years ago
Posted patch v1.5 patchSplinter Review
revised to load getaddrinfo/freeaddrinfo properly on WinXP.  this patch also
includes the last two attachments for OpenVMS and OS/2.
(Assignee)

Updated

16 years ago
Attachment #127743 - Attachment is obsolete: true
Attachment #128152 - Attachment is obsolete: true
Attachment #128160 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #127743 - Flags: review?(wtc)
(Assignee)

Comment 21

16 years ago
Comment on attachment 128183 [details] [diff] [review]
v1.5 patch

wtc: this patch does not include making PR_GetAddrInfoByName return PRAddrInfo*
  ... in case you have other comments, i'd prefer to fix all together.
Attachment #128183 - Flags: review?(wtc)

Comment 22

16 years ago
Darin, I made the first pass through your patch.  I did some
editing and checked in your patch on the NSPR_GETADDRINFO_BRANCH
of mozilla/nsprpub.

I have not made any API changes, so your code should compile and
link just fine against NSPR_GETADDRINFO_BRANCH.  I did make a
semantic change to PR_EnumerateAddrInfo: PR_EnumerateAddrInfo
now returns NULL when there is no more address to enumerate, so
you should not use the address when PR_EnumerateAddrInfo returns
NULL.  This allows you to write the enumeration using a while
loop like this:

    void *iter;
    PRAddrInfo *ai;
    PRNetAddr addr;
    PRUint16 port;

    iter = NULL;
    while ((iter = PR_EnumerateAddrInfo(iter, ai, port, &addr)) != NULL) {
        /* use 'addr' */
    }

You'd need to modify your patches in bug 205726 accordingly.  The
reason for this change is to be consistent with PR_EnumerateHostEnt.
Let me know if you like this change.

Comment 23

16 years ago
I also changed the meaning of the HAVE_GETADDRINFO
macro (it's also renamed _PR_HAVE_GETADDRINFO).
It used to mean "the target OS has get getaddrinfo";
it now means "the current or a later version of the
target OS has getaddrinfo".  Again, this is to be
consistent with the meaning of _PR_HAVE_GETIPNODEBYNAME.
(Assignee)

Comment 24

16 years ago
wtc: i took a look at the changes on the branch, and they look great.  i really
like the way you cleaned up the #defines :)


>I have not made any API changes, so your code should compile and
>link just fine against NSPR_GETADDRINFO_BRANCH.  

but do you still want to make PR_GetAddrInfoByName return |PRAddrInfo*|?


>I did make a semantic change to PR_EnumerateAddrInfo: PR_EnumerateAddrInfo
>now returns NULL when there is no more address to enumerate, so
>you should not use the address when PR_EnumerateAddrInfo returns NULL.

this is what i have been implementing or so i thought.  was there a case where
this was somehow not true (in the v1.5 patch)?


also, let me know what you think remains to be done with this patch.  thanks!

Comment 25

16 years ago
Is this getting checked in for 1.5?
(Assignee)

Comment 26

16 years ago
colin: no, i've decided to postpone the DNS rewrite until the trunk opens for
1.6 alpha.  reviews on the necko portion are still pending :(
(Assignee)

Comment 27

16 years ago
ok, this is getting checked in shortly.  wtc gave me approval yesterday to land
this on the NSPRPUB_PRE_4_2_CLIENT_BRANCH and on the NSPR trunk.
(Assignee)

Comment 28

16 years ago
ok, this is now checked in on the NSPR trunk and NSPRPUB_PRE_4_2_CLIENT_BRANCH.
wtc says that there are likely build problems on OSF1 that we will need to fix.

if anyone happens to have access to an OSF1 box, please let us know!
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Updated

16 years ago
Blocks: 175340

Updated

16 years ago
Whiteboard: [4.5]
Filed bug 219104 on a Mac OS X issue in this patch.
I've yet to verify it, but this checkin might have caused a problem in Windows
98.  Entering an ip address instead of dns name causes a "could not be found" error.

Comment 31

16 years ago
The problem you described in comment 30 is introduced by
this checkin and is the subject of bug 219376.

Updated

16 years ago
Attachment #128183 - Flags: review?(wchang0222)

Updated

16 years ago
Whiteboard: [4.5]
Target Milestone: --- → 4.5
You need to log in before you can comment on or make changes to this bug.