71697 - [patch] Changes for NSPR on BeOS/BONE

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement, P2)

Description

[Matthew Zahorik]

These patches are necessary to build the NSPR under the in-beta next-gen 
networking stack for BeOS, known as BONE.  It more closely conforms to the BSD 
socket API and brings BeOS' networking performance to par with Linux and *BSD 
implementations.

All patches are in BeOS-specific code EXCEPT for the following:

pr/include/prio.h      This is particularly messy and needs careful review.
                       BeOS/non-BONE is different from UNIX sockaddr_in, and
                       BeOS/BONE is once again different from both.
pr/src/io/prfile.c     This only adds an XP_BEOS to enable pipes on BeOS
pr/src/io/prmapopt.c   Adds a defined(BONE_VERSION) to include netinet/tcp.h
                       and add PR_Sockopt_Linger support back in.
pr/src/io/prsocket.c   If BONE_VERSION is defined pass osfd to
                       _MD_beos_get_nonblocking_connect_error instead of FD.
pr/src/misc/prnetdb.c  If XP_BEOS and BONE_VERSION are defined, include the
                       system file arpa/inet.h (thus defining inet_ntoa)
configure.in           If in *beos) check for libbind.so, and if it exists set
                       OS_LIBS to "$OS_LIBS -lbind -lsocket"

These patches retain the ability to compile the NSPR on non-BONE BeOS machines, 
and also shouldn't interfere with other platforms.  But I don't have access to 
these other platforms, so please carefully review the XP code changes.

You also need patch #71681 if you'd like to build mozilla under BeOS/BONE plus 
an upcoming patch to mozilla/configure.in necessary to include -lbind and -
lsocket just like in the NSPR.

Thanks!

- Matt

Comment 1

[Matthew Zahorik]

Attachment: Diffs for BeOS/BONE to NSPR

Comment 2

[cls]

Matt, is there any chance you can repost the patch using a unified (-u) diff?  I
find that it usually makes things much clearer and I probably wouldn't have
immediately gone into shock at the apparent removal of the non-BeOS PRNetAddr 
.raw.family variable. :-)

Although it's probably unlikely that any other platform would define
BONE_VERSION, it's a generic enough term that I'd request that you make that
BONE_VERSION ifdefs are contained inside or accompany a XP_BEOS ifdef. Unless
the ifdef is in a BeOS specific file, of course. nsprpub/pr/src/io/prmapopt.c is
a trouble spot I saw.

Larry, any further thoughts?

Comment 3

[Matthew Zahorik]

I'll do items #1 and #2 in the next couple of days.

pr/include/prio.h is my item of major concern - it's quite messy.  Is there an 
NSPR-approved way to break that out?

#ifdef XP_BEOS
#include <beos_netaddr.h>
#else /* XP_BEOS */
.... UNIX PRNetAddr
#endif

etc..

Comment 4

[larryh (gone)]

needs some more review.

Comment 5

[Wan-Teh Chang]

Matthew,

Is your patch still good (which is more than three
months old now) or do you have a new one?

Comment 6

[larryh (gone)]

Reassign to wtc.

Comment 7

[cls]

Attachment: _MD_pr_poll implementation extracted from Matt's patch that works for non-BONE too.

Comment 8

[Daniel Switkin]

Does anyone have a more up-to-date patch (i.e. BONE/7a for Mozilla 0.9.7)?

Comment 9

[Wan-Teh Chang]

Assigned the bug to Chris.

Comment 10

[Paul]

Chris, I just applied the latest patch on this bug, because of comments in
Bug#70808.  Well, from preliminary testing, it seems that this patch fixes
Bug#100508.  This is a very good thing!  What are the chances of getting this
checked in?  

I am addind [e-mail] to CC list to test this as well.

Comment 11

[Paul]

Well, can't add Daniel, I'll email him directly (he's not a member).

Comment 12

[hacker formerly known as [e-mail]]

I'm surprised that this patch alone would fix the page loading problem.  I've
cleaned my tree since then so I'll have to reapply the patch and test it. 
Matt's original patch needs to be cleaned up and tested by someone with BONE access.

Comment 13

[Paul]

I just made a BONE build last night.  I should be able to update the patch.  

I wasn't sure it would work, but what lead me to this was the comments on
Bug#70808.  It just seemed to me that the problems were related, since the
problems with the pages was more than likely due to the fact that threads and
the net_server to not work well, supposedly.  I thought I'd just give it a try,
and the results so far are promising.

Comment 14

[Paul]

Attachment: Update bone patch

This patch contains the changes from the previous two patchs, plus some other
modifications.	Everything seems to work fine, except for the build.  I can't
seem to get the generation of nsprpub/config/autoconf.mk to include OS_LIBS=
-lbind -lsocket.  If you apply this patch, once configure is finished, you'll
need to make this change manually, to get mozilla to compile.  Someone who
knows a little more about the build process should be able to fix this easily.

Comment 15

[Paul]

Oh, the above patch has not been tested under net_server, yet.

Comment 16

[hacker formerly known as [e-mail]]

Comment on attachment 63025 [details] [diff] [review]
Update bone patch

For the AC_CHECK_LIB macro, you probably want to use some function that's
actually in the bind library.  I don't think it contains main().  

I thought that beos did have support for non-blocking pipes?  IIRC, it's just
not hooked up properly as some places in the code call _MD_MakeNonBlock
directly instead of _MD_MAKE_NONBLOCK and beos uses _MD_make_nonblock.	

This patch ignores the changes from attachment 46653 [details] [diff] [review] which uses a single
_MD_pr_poll implementation for both builds.

Comment 17

[hacker formerly known as [e-mail]]

Oops. Ignore my previous rattlings about non-blocking pipes.  I was thinking
about non-blocking sockets.  

Comment 18

[hacker formerly known as [e-mail]]

Attachment: BONE patch + non-BONE merge changes

Here's the BONE patch with the changes that I mentioned previously.  It uses
the same _MD_pr_poll implementation for BONE & net_server builds.  It also uses
the same socket_io_wait implementation for both builds.  Looking at the
previous patch, the only difference between the 2 impls (minus whitespace
changes) was the handling of a _MD_SELECT error condition.  The patch has only
been tested on a net_server build.

Comment 19

[Wan-Teh Chang]

Comment on attachment 63053 [details] [diff] [review]
BONE patch + non-BONE merge changes

>Index: nsprpub/pr/include/prio.h
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/pr/include/prio.h,v
>retrieving revision 3.31
>diff -u -r3.31 prio.h
>--- nsprpub/pr/include/prio.h	1 Sep 2000 06:02:00 -0000	3.31
>+++ nsprpub/pr/include/prio.h	30 Dec 2001 06:30:00 -0000
>@@ -162,25 +162,46 @@
> 
> union PRNetAddr {
>     struct {
>-        PRUint16 family;                /* address family (0x00ff maskable) */
> #ifdef XP_BEOS
>-        char data[10];                  /* Be has a smaller structure */
>-#else
>-        char data[14];                  /* raw address data */
>-#endif
>+#ifdef BONE_VERSION
>+	PRUint8  len;		/* BeOS w/ BONE has a 32-byte sockaddr */
>+	PRUint8	 family;	/* Plus a "length" entry */
>+	char	 data[30];
>+#else /* BONE_VERSION */
>+	PRUint16 family;	/* BeOS w/o BONE is much smaller at 12 */
>+	char	 data[10];
>+#endif /* BONE_VERSION */
>+#else /* XP_BEOS */
>+        PRUint16 family;        /* address family (0x00ff maskable) */
>+        char data[14];          /* raw address data */
>+#endif /* XP_BEOS */
>     } raw;
>     struct {
>+#if defined(XP_BEOS) && defined(BONE_VERSION)
>+	PRUint8	 len;
>+	PRUint8	 family;
>+#else /* XP_BEOS && BONE_VERSION */
>         PRUint16 family;                /* address family (AF_INET) */
>+#endif /* XP_BEOS && BONE_VERSION */
>         PRUint16 port;                  /* port number */
>         PRUint32 ip;                    /* The actual 32 bits of address */
> #ifdef XP_BEOS
>-        char pad[4];                    /* Be has a smaller structure */
>-#else
>+#ifdef BONE_VERSION
>+	char pad[24];		/* Bring total to 32 bytes for BONE */
>+#else /* BONE_VERSION */
>+	char pad[4];		/* BeOS w/o BONE is only 12 bytes */
>+#endif /* BONE_VERSION */
>+#else /* XP_BEOS */
>         char pad[8];
>-#endif
>+#endif /* XP_BEOS */
>     } inet;
>     struct {
>+#if defined(XP_BEOS) && defined(BONE_VERSION)
>+	PRUint8	 len;
>+	PRUint8	 family;
>+#else
>         PRUint16 family;                /* address family (AF_INET6) */
>+#endif
>         PRUint16 port;                  /* port number */
>         PRUint32 flowinfo;              /* routing information */
>         PRIPv6Addr ip;                  /* the actual 128 bits of address */

PRNetAddr needs to be as platform-independent as possible.
The correct way to deal with the 'len' field in
struct sockaddr_in is to define the _PR_HAVE_SOCKADDR_LEN
macro.	There are examples in mozilla/nsprpub/pr/include/md/*.h.
It seems that adding the following to
mozilla/nsprpub/pr/include/md/_beos.h would work:

#ifdef BONE_VERSION
#define _PR_HAVE_SOCKADDR_LEN
#endif

In nsprpub/pr/include/md/_beos.h:

>+#ifdef BONE_VERSION
>+NSPR_API(PRInt32) _MD_writev(PRFileDesc *fd, const PRIOVec *iov, PRInt32 iov_size, PRIntervalTime timeout);
>+#else
> NSPR_API(PRInt32) _MD_writev(PRFileDesc *fd, struct PRIOVec *iov, PRInt32 iov_size, PRIntervalTime timeout);
>+#endif

We should just use the first prototype (with const PRIOVec *iov)
and not check the BONE_VERSION macro at all.

>Index: nsprpub/pr/src/io/prmapopt.c
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/pr/src/io/prmapopt.c,v
>retrieving revision 3.14
>diff -u -r3.14 prmapopt.c
>--- nsprpub/pr/src/io/prmapopt.c	20 Jun 2000 21:28:18 -0000	3.14
>+++ nsprpub/pr/src/io/prmapopt.c	30 Dec 2001 06:30:01 -0000
>@@ -86,7 +86,7 @@
>         {
>             case PR_SockOpt_Linger:
>             {
>-#if !defined(XP_BEOS)
>+#if !defined(XP_BEOS) || (defined(XP_BEOS) && defined(BONE_VERSION))
>                 struct linger linger;
>                 length = sizeof(linger);
>                 rv = _PR_MD_GETSOCKOPT(
>@@ -244,7 +244,7 @@
>         {
>             case PR_SockOpt_Linger:
>             {
>-#if !defined(XP_BEOS)
>+#if !defined(XP_BEOS) || (defined(XP_BEOS) && defined(BONE_VERSION))
>                 struct linger linger;
>                 linger.l_onoff = data->value.linger.polarity;
>                 linger.l_linger = PR_IntervalToSeconds(data->value.linger.linger);

A nit: the ifdef expressions can be simplified as
#if !defined(XP_BEOS) || defined(BONE_VERSION)

>Index: nsprpub/pr/src/md/beos/bfile.c

>-PRInt32
>-_MD_pr_poll (PRPollDesc *pds, PRIntn npds, PRIntervalTime timeout)
>+static PRInt32 NativeThreadSelect( PRPollDesc *pds, PRIntn npds, 
>+				   PRIntervalTime timeout)

Since we always use native threads on BeOS, it is not
necessary to have a separate NativeThreadSelect() function.
The NativeThreadSelect() function should simply be part of
the _MD_pr_poll function.

>+PRInt32 _MD_pr_poll(PRPollDesc *pds, PRIntn npds, PRIntervalTime timeout)
>+{
>+	PRInt32 rv = 0;
>+	PRThread *me = _PR_MD_CURRENT_THREAD();
>+
>+	if (_PR_PENDING_INTERRUPT(me))
>+	{
>+		me->flags &= ~_PR_INTERRUPT;
>+		PR_SetError(PR_PENDING_INTERRUPT_ERROR, 0);
>+		return -1;
>+	}
>+
>+	if (0 == npds) PR_Sleep(timeout);
>+	rv = NativeThreadSelect(pds, npds, timeout);
>+	return rv;
>+}  /* _MD_pr_poll */

We should return rv right after the PR_Sleep() call.

>Index: nsprpub/pr/src/md/beos/bnet.c

Make sure you delete the debug printf statements.

All the other changes look fine, although I didn't
review all of them carefully.

Thank you, Chris, for working on this bug.

Comment 20

[hacker formerly known as [e-mail]]

Attachment: updated patch with wtc's suggested changes

Wan-Teh,  this updated patch has all of the changes that you suggested except
the PR_HAVE_SOCKADDR_LEN change.   A quick lxr shows that define is only used
by the pthreads & unix code, neither of which BeOS uses.  Since the size of the
various structs inside the PRNetAddr union is different for BONE & net_server
builds, I don't see how using that define would have simplified that ifdef
mess.

Comment 21

[Wan-Teh Chang]

Chris, the purpose of the _PR_HAVE_SOCKADDR_LEN macro is not to
simplify the ifdef mess.  In fact, it may require adding more
ifdefs to the implementation.  Our goal is that the definition
of PRNetAddr should be as platform-neutral as possible.

There is some value in making PRNetAddr be binary compatible
with struct sockaddr_in, but that is not required.  We want
to ensure that an NSPR client never needs to set the 'len'
field of PRNetAddr.

This means that you will need to duplicate the _PR_HAVE_SOCKADDR_LEN
handling from the Unix/pthreads code to either prsocket.c or
the BeOS socket function implementation.

Comment 22

[Wan-Teh Chang]

Comment on attachment 63232 [details] [diff] [review]
updated patch with wtc's suggested changes

I must reject the proposed changes to prio.h.  Sorry.

Comment 23

[hacker formerly known as [e-mail]]

Attachment: updated patch + wtc changes - prio.h change

Same patch with the prio.h chunk taken out.   Rewriting the beos socket
implementation may take some time (I haven't looked yet) so I don't want to
hold up the rest of that patch for it.

Comment 24

[Wan-Teh Chang]

Comment on attachment 63357 [details] [diff] [review]
updated patch + wtc changes - prio.h change

1. mozilla/nsprpub/pr/src/io/prfile.c

The change from _MD_MakeNonblock to _MD_MAKE_NONBLOCK
is good.  It would be even better to change it to
_PR_MD_MAKE_NONBLOCK.  (See mozilla/nsprpub/pr/src/io/prsocket.c
for examples.)

2. mozilla/nsprpub/pr/src/md/beos/bnet.c

The debug printf statements in this file should either
be deleted or replaced by PR_LOG statements.

3. mozilla/nsprpub/pr/tests/affinity.c

It's better to declare the main function as
    int main()
and have it explicitly return 0.

Fix these and you can go ahead and check this in.
Thanks!

Comment 25

[Paul]

Attachment: updated patch + wtc suggestions - prio.h

Updated patch.	Fixes according to wtc's comments, plus AC_CHECK_LIB in
configure.in fix.  

BeOS does not have autoconf by default, but is available,
and is needed for a BONE build.  Being as BONE is beta software, this should
not matter.

Not having the changes to prio.h does not seem to affect mozilla under BONE.
I have not seen any problems with the BONE build since this patch was
applied.  Non-BONE, net_server builds, are working as well.

Comment 26

[hacker formerly known as [e-mail]]

Attachment 64218 [details] [diff] has been checked in on the NSPR tip and the
NSPRPUB_PRE_4_2_CLIENT_BRANCH.  Marking fixed.


Checking in nsprpub//configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.83.2.15; previous revision: 1.83.2.14
done
Checking in nsprpub//pr/include/md/_beos.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_beos.h,v  <--  _beos.h
new revision: 3.17.2.1; previous revision: 3.17
done
Checking in nsprpub//pr/src/io/prfile.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prfile.c,v  <--  prfile.c
new revision: 3.36.2.2; previous revision: 3.36.2.1
done
Checking in nsprpub//pr/src/io/prmapopt.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prmapopt.c,v  <--  prmapopt.c
new revision: 3.14.4.1; previous revision: 3.14
done
Checking in nsprpub//pr/src/md/beos/bfile.c;
/cvsroot/mozilla/nsprpub/pr/src/md/beos/bfile.c,v  <--  bfile.c
new revision: 3.7.2.1; previous revision: 3.7
done
Checking in nsprpub//pr/src/md/beos/bnet.c;
/cvsroot/mozilla/nsprpub/pr/src/md/beos/bnet.c,v  <--  bnet.c
new revision: 3.5.4.1; previous revision: 3.5
done
Checking in nsprpub//pr/src/misc/prnetdb.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prnetdb.c,v  <--  prnetdb.c
new revision: 3.21.2.2; previous revision: 3.21.2.1
done
Checking in nsprpub//pr/tests/affinity.c;
/cvsroot/mozilla/nsprpub/pr/tests/affinity.c,v  <--  affinity.c
new revision: 3.4.4.1; previous revision: 3.4
done
Checking in nsprpub/configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.78.2.15; previous revision: 1.78.2.14
done
$                                                     

Checking in nsprpub//configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.97; previous revision: 1.96
done
Checking in nsprpub//pr/include/md/_beos.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_beos.h,v  <--  _beos.h
new revision: 3.18; previous revision: 3.17
done
Checking in nsprpub//pr/src/io/prfile.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prfile.c,v  <--  prfile.c
new revision: 3.37; previous revision: 3.36
done
Checking in nsprpub//pr/src/io/prmapopt.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prmapopt.c,v  <--  prmapopt.c
new revision: 3.15; previous revision: 3.14
done
Checking in nsprpub//pr/src/md/beos/bfile.c;
/cvsroot/mozilla/nsprpub/pr/src/md/beos/bfile.c,v  <--  bfile.c
new revision: 3.8; previous revision: 3.7
done
Checking in nsprpub//pr/src/md/beos/bnet.c;
/cvsroot/mozilla/nsprpub/pr/src/md/beos/bnet.c,v  <--  bnet.c
new revision: 3.6; previous revision: 3.5
done
Checking in nsprpub//pr/src/misc/prnetdb.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prnetdb.c,v  <--  prnetdb.c
new revision: 3.25; previous revision: 3.24
done
Checking in nsprpub//pr/tests/affinity.c;
/cvsroot/mozilla/nsprpub/pr/tests/affinity.c,v  <--  affinity.c
new revision: 3.5; previous revision: 3.4
done
Checking in nsprpub/configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.92; previous revision: 1.91
done
$                                               

Comment 27

[Wan-Teh Chang]

Comment on attachment 64218 [details] [diff] [review]
updated patch + wtc suggestions - prio.h

This patch is good.  r=wtc.

Thank you, Chris and Arougthopher!