[patch] Changes for NSPR on BeOS/BONE

RESOLVED FIXED in 4.2

Status

P2
enhancement
RESOLVED FIXED
18 years ago
17 years ago

People

(Reporter: maz, Assigned: netscape)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

18 years ago
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
(Reporter)

Comment 1

18 years ago
Created attachment 27468 [details] [diff] [review]
Diffs for BeOS/BONE to NSPR

Updated

18 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

18 years ago
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?


(Reporter)

Comment 3

18 years ago
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..

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 4

18 years ago
needs some more review.
Priority: -- → P2
Target Milestone: --- → 4.2

Updated

18 years ago
Priority: P2 → P1

Comment 5

18 years ago
Matthew,

Is your patch still good (which is more than three
months old now) or do you have a new one?
Keywords: mozilla0.9.3, patch, review

Comment 6

18 years ago
Reassign to wtc.
Assignee: larryh → wtc
Status: ASSIGNED → NEW

Comment 7

17 years ago
Created attachment 46653 [details] [diff] [review]
_MD_pr_poll implementation extracted from Matt's patch that works for non-BONE too.

Comment 8

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

Comment 9

17 years ago
Assigned the bug to Chris.
Assignee: wtc → seawood
Priority: P1 → P2
Target Milestone: 4.2 → Future
Status: NEW → ASSIGNED

Comment 10

17 years ago
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 daniel@switkin.com to CC list to test this as well.

Comment 11

17 years ago
Well, can't add Daniel, I'll email him directly (he's not a member).
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

17 years ago
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

17 years ago
Created attachment 63025 [details] [diff] [review]
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.
Attachment #27468 - Attachment is obsolete: true

Comment 15

17 years ago
Oh, the above patch has not been tested under net_server, yet.
Attachment #63025 - Flags: needs-work+
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.
Oops. Ignore my previous rattlings about non-blocking pipes.  I was thinking
about non-blocking sockets.  
Keywords: review
QA Contact: larryh → wtc
Target Milestone: Future → 4.2
Created attachment 63053 [details] [diff] [review]
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.
Attachment #46653 - Attachment is obsolete: true
Attachment #63025 - Attachment is obsolete: true

Comment 19

17 years ago
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.
Attachment #63053 - Flags: needs-work+
Created attachment 63232 [details] [diff] [review]
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.
Attachment #63053 - Attachment is obsolete: true

Comment 21

17 years ago
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

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

I must reject the proposed changes to prio.h.  Sorry.
Attachment #63232 - Flags: needs-work+
Created attachment 63357 [details] [diff] [review]
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.
Attachment #63232 - Attachment is obsolete: true

Comment 24

17 years ago
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!
Attachment #63357 - Flags: needs-work+

Comment 25

17 years ago
Created attachment 64218 [details] [diff] [review]
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.
Attachment #63357 - Attachment is obsolete: true
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
$                                               

Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 27

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

This patch is good.  r=wtc.

Thank you, Chris and Arougthopher!
Attachment #64218 - Flags: review+
You need to log in before you can comment on or make changes to this bug.