773959 - Fix mismatch of PRNetAddr and sockaddr on OS/2

Status: NEW

(NSPR :: NSPR, defect)

Description

[KO Myung-Hun]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0.1
Build ID: 20120614114901

Comment 1

[KO Myung-Hun]

OS/2 32-bits TCP/IP puts 1 byte length field and 1 byte family field at first. But PRNetAddr puts 2 bytes family field at first.

Comment 2

[KO Myung-Hun]

Attachment: Fix mismatch of PRNetAddr and sockaddr

Comment 3

[Dave Yeo]

IIRC nsprpub patches must be cvs patches against nsprpub trunk but I don't remember the depositories URL.
Walter or Peter, do you remember?
While the patch is pretty well correct, I think it should be expanded to also remove TCPV40HDRS from the build system. The 32 bit stack is freely available and I believe most users have updated their stack or updated to eCS.
Once the patch[es} are finalized, review will have to be requested from one of the nsprpub maintainers.

Comment 4

[KO Myung-Hun]

I think removing TCPV40HDRS from the build system is another problem.

So we should open another ticket for it after this patch is landed.

And provided you tell me the URL for nspurpub, I'll do that.

Comment 5

[Peter Weilbacher]

<https://developer.mozilla.org/en/NSPR_build_instructions> has info on CVS access and build of NSPR.

Comment 6

[Dave Yeo]

Comment on attachment 642226 [details] [diff] [review]
Fix mismatch of PRNetAddr and sockaddr

While the patch works to fix the 100% CPU issue and seems correct I don't have review rights so changing the requestee to Wan-Teh Chang.
Not sure if you should be doing a cvs diff from cvs-mirror.mozilla.org or if they followed through on their intention to turn off write access to cvs in the 2nd quarter of this year.

Comment 7

[Wan-Teh Chang]

Comment on attachment 642226 [details] [diff] [review]
Fix mismatch of PRNetAddr and sockaddr

Review of attachment 642226 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch.  I suggest an alternative fix below.

::: nsprpub/pr/include/prio.h
@@ +145,5 @@
>  union PRNetAddr {
>      struct {
> +#if defined(XP_OS2) && !defined(TCPV40HDRS)
> +        PRUint8 len;
> +        PRUint8 family;

We should not change PRNetAddr like this.  We want PRNetAddr
to be as cross-platform as possible.

The correct fix is as follows.

1. Add the following to mozilla/nsprpub/pr/include/md/_os2.h:

#if !defined(TCPV40HDRS)
#define _PR_HAVE_SOCKADDR_LEN
#endif

2. In the OS/2 socket code, convert PRNetAddr to struct sockaddr
if _PR_HAVE_SOCKADDR_LEN is defined.  You can see how this is done
for Unix in mozilla/nsprpub/pr/src/pthreads/ptio.c.  The following
IO methods need special handing for _PR_HAVE_SOCKADDR_LEN:

  bind
  accept
  connect
  recvfrom
  sendto
  getsockname
  getpeername

Sorry for the trouble, but all this complexity is needed to
avoid exposing a platform-dependent 'len' field in PRNetAddr.

Question: Is Mozilla still accepting patches for OS/2?  At some
point we'll need to drop OS/2 support.

Comment 8

[Steve Wendt]

(In reply to Wan-Teh Chang from comment #7)
> Question: Is Mozilla still accepting patches for OS/2?  At some
> point we'll need to drop OS/2 support.

Why is that?

Comment 9

[Dave Yeo]

(In reply to Wan-Teh Chang from comment #7)
[...]
> 
> Question: Is Mozilla still accepting patches for OS/2?

Yes

>  At some point we'll need to drop OS/2 support.

OS/2 is still being sold as an OEM version, http://ecomstation.com/ and updated to work on modern hardware though you have to be picky about hardware.

Comment 10

[KO Myung-Hun]

Attachment: v2 for PRNetAddr

Comment 11

[KO Myung-Hun]

Ping ?

Comment 12

[KO Myung-Hun]

Ping2 ?

Comment 13

[KO Myung-Hun]

Not acceptable ?