Closed Bug 729511 Opened 12 years ago Closed 12 years ago

Import SCTP library from FreeBSD

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: jesup, Assigned: jesup)

References

Details

Attachments

(2 files, 48 obsolete files)

2.66 MB, patch
Details | Diff | Splinter Review
9.58 KB, patch
ted
: review+
Details | Diff | Splinter Review
Import libsctp from the FreeBSD implementation (already broken out as a user library).  Contacts are Michael Tuexen, Randall Stewart (SCTP author) and Salvatore Loreto (of Ericsson); they're willing to make mods if we need them.
Blocks: 729512
cc-ing Jonathan, who worked on bug 383180 and might be interested to know what we're doing here.
First-cut patch to import SCTP.  To import sctp (with this patch applied):

First, check out SCTP:
# sctp usrlib source is available (via cvs) at:
# :ext:anoncvs@stewart.chicago.il.us:/usr/sctpCVS
# password: sctp

The do the import:

mkdir network/protocol/sctp/src/netinet
mkdir network/protocol/sctp/src/netinet6
netwerk/protocol/sctp/update_sctp.sh   dir_where_sctp_KERN_is
hg qnew sctp_files 

There is one fix currently: line 5782 of netinet/sctputil.c needs to filter out linux as well (bug report sent upstream).
Oh, just to note: while they haven't finished packaging it up, the userspace libsctp should be BSD-licensed (per the norm, since it comes from FreeBSD).
updated integration patch - move to netwerk out of protocols
Attachment #608436 - Attachment is obsolete: true
Attachment #607459 - Attachment is obsolete: true
Attached patch fixes and updates to libsctp (obsolete) — Splinter Review
many of these will go away with coming interface changes to userland libsctp
Attachment #608762 - Attachment is obsolete: true
Attachment #610674 - Attachment is obsolete: true
Depends on: 744895
Attachment #611739 - Attachment is obsolete: true
Attachment #611929 - Attachment is obsolete: true
Attachment #610691 - Attachment is obsolete: true
Attachment #610673 - Attachment is obsolete: true
Attachment #614897 - Attachment is obsolete: true
Apply base library, then "fixes and updates", then "build within the Mozilla tree".  All of these should be basically identical between alder/default and paris_demo
Attachment #608763 - Attachment is obsolete: true
Attachment #617793 - Attachment is obsolete: true
Attachment #617792 - Attachment is obsolete: true
These are the updated DataChannel drops for the new top-level SCTP API (with proper licensing, etc).  This builds on Windows, but doesn't seem to run (there's fancy footwork going on to make it work on WinXP...).
Attached patch Update sctp for Windows XP (obsolete) — Splinter Review
Blocks: dc-tests
Attachment #634382 - Attachment is obsolete: true
Attachment #634384 - Attachment is obsolete: true
Attachment #634385 - Attachment is obsolete: true
Attachment #640920 - Attachment is obsolete: true
Attachment #640921 - Attachment is obsolete: true
Attachment #640922 - Attachment is obsolete: true
Attachment #634393 - Attachment is obsolete: true
Attachment #641696 - Attachment is obsolete: true
Attachment #641697 - Attachment is obsolete: true
Attachment #641698 - Attachment is obsolete: true
Attached patch Update MPL headers (obsolete) — Splinter Review
Comment on attachment 649080 [details] [diff] [review]
Import updated sctp library as of 2012/8/5 3:51am EDT

Current tests (xpcshell raw_xpcshell_data.js) work fine with this change.

Had to add a #include sctp_constants.h if SCTP_DEBUG is on for SCTP_DEBUG_ALL, etc

Checked into alder:
http://hg.mozilla.org/projects/alder/rev/f20dfda693fe
This is an update of jesup's SCTP patch, these are all the changes to SCTP upstream I needed to get it to compile on Windows.
Attachment #652090 - Attachment is obsolete: true
One additional change to jesup's DataChannel patch to fix the SCTP stdint issue on Windows.

With both of these patches applied I can build netwerk/sctp on Windows.
Attachment #652119 - Attachment is obsolete: true
current base patch for netwerk/sctp/src for m-c
Attachment #645448 - Attachment is obsolete: true
current mods to usrsctp library (proposed upstream patch)
Attachment #651765 - Attachment is obsolete: true
Attachment #645462 - Attachment is obsolete: true
Attachment #649080 - Attachment is obsolete: true
Attachment #650620 - Attachment is obsolete: true
Attachment #655276 - Attachment is obsolete: true
Attachment #655276 - Attachment is obsolete: false
Attachment #650612 - Attachment is obsolete: true
Attachment #651845 - Attachment is obsolete: true
Attachment #652130 - Attachment is obsolete: true
removed Makefile.in, which shouldn't have been included
Attachment #655403 - Attachment is obsolete: true
Attachment #645463 - Attachment is obsolete: true
Attachment #655452 - Attachment is obsolete: true
Attachment #645478 - Attachment is obsolete: true
Attachment #649213 - Attachment is obsolete: true
Attachment #651756 - Attachment is obsolete: true
Attachment #652131 - Attachment is obsolete: true
Attachment #655276 - Attachment is obsolete: true
Attachment #655453 - Attachment is obsolete: true
Comment on attachment 656168 [details] [diff] [review]
rebuilt import of usrsctp library

Looking for reviews on the SCTP library portion of datachannels.  There are 3 bugs here: the import itself, a patch intended for upstreaming (which they're already looking at), and a patch to make it build in our tree (Makefile.in's, configure.in, etc.  The will come bug 729512 (DataChannel protocol), and bug 733002 (DataChannel DOM).

Once they update the library to resolve the issues in the patch to their lib, we expect to continue with no mods to the imported files.
Attachment #656168 - Flags: review?(mcmanus)
Attachment #656168 - Flags: review?(cbiesinger)
Attachment #655404 - Attachment is obsolete: true
Attachment #656179 - Flags: review?(mcmanus)
Attachment #656179 - Flags: review?(cbiesinger)
Comment on attachment 655519 [details] [diff] [review]
build libsctp within Mozilla tree

Patch applies to m-c with a few line offsets - build SCTP within the mozilla system.  patch 3 of 3 to get the SCTP protocol building.
Attachment #655519 - Flags: review?(mcmanus)
Attachment #655519 - Flags: review?(cbiesinger)
From Randall Stewart (SCTP author):

The project is accessed via:

svn co --username your.user.name https://sctp-refimpl.googlecode.com/svn/trunk sctpSVN

If you are just wanting a read-only copy use

svn co --username your.user.name http://sctp-refimpl.googlecode.com/svn/trunk sctpSVN
Comment on attachment 655519 [details] [diff] [review]
build libsctp within Mozilla tree

Adding ted to review the build system stuff for SCTP (which is most of this patch; patrick and biesi may want to defer on it to him)
Attachment #655519 - Flags: review?(ted.mielczarek)
Comment on attachment 655519 [details] [diff] [review]
build libsctp within Mozilla tree

+++ b/netwerk/sctp/sctp_update.log

is it useful to have the references to your local directory in this file?

+++ b/netwerk/sctp/src/Makefile.in
+#EXPORTS_mozilla/net/netinet = \

Why's this commented out? Can you either add a comment stating why, or remove it?

+DEFINES_Debug += \
+   $(NULL)

just remove this. (I'll note that DEFINES_debug below has lowercase debug...)

+++ b/netwerk/sctp/update_sctp.sh
+# sctp usrlib source is available (via cvs) at:
+# :ext:anoncvs@stewart.chicago.il.us:/usr/sctpCVS
+# password: sctp

should be updated with the new URL you mentioned in the comment

+# hand after this process, or use a more complex on.

on -> one


Can you add a README in netwerk/sctp that this is upstream library and mention its URL?

I wish we had a better place for third-party libraries other than dumping them in random locations in our source tree, indistinguishable from our own code...
Attachment #655519 - Flags: review?(cbiesinger) → review-
Comment on attachment 656168 [details] [diff] [review]
rebuilt import of usrsctp library

this is just the output of the update_sctp.sh script, unmodified from upstream, right? r=biesi if so
Attachment #656168 - Flags: review?(cbiesinger) → review+
Attachment #656179 - Flags: review?(cbiesinger) → review+
Attachment #656168 - Attachment is obsolete: true
Attachment #656168 - Flags: review?(mcmanus)
(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #58)
> Comment on attachment 655519 [details] [diff] [review]
> build libsctp within Mozilla tree
> 
> +++ b/netwerk/sctp/sctp_update.log
> 
> is it useful to have the references to your local directory in this file?

Nope.  Revised script to say "Updated from SVN version XXXX on YYYYY"; edited import file

> 
> +++ b/netwerk/sctp/src/Makefile.in
> +#EXPORTS_mozilla/net/netinet = \
> 
> Why's this commented out? Can you either add a comment stating why, or
> remove it?

That was needed until they did a revision to clean up the "external" include (usrsctp.h).  Removed.

> 
> +DEFINES_Debug += \
> +   $(NULL)
> 
> just remove this. (I'll note that DEFINES_debug below has lowercase debug...)

Cruft, removed.

> +++ b/netwerk/sctp/update_sctp.sh
> +# sctp usrlib source is available (via cvs) at:
> +# :ext:anoncvs@stewart.chicago.il.us:/usr/sctpCVS
> +# password: sctp
> 
> should be updated with the new URL you mentioned in the comment

Done.

> +# hand after this process, or use a more complex on.
> 
> on -> one

Done, and added comments about how one would do that if needed.

> Can you add a README in netwerk/sctp that this is upstream library and
> mention its URL?
> 
> I wish we had a better place for third-party libraries other than dumping
> them in random locations in our source tree, indistinguishable from our own
> code...

Added.  And I agree...  or at least a standard nomenclature like "external/" or "imported/" (netwerk/sctp/src/imported or netwerk/sctp/imported/src, or imported/netwerk/sctp).  But it makes little sense to do it for one module and not the entire tree.  Another mass-rename/re-org, though I doubt this one will ever happen.  Even a top-level file listing what we've imported, what the license is, what the update policy and procedure is, etc would be a HUGE improvement.... I may suggest that to Gerv.
Comment on attachment 656712 [details] [diff] [review]
import of usrsctp library svn rev 8119 r=biesi

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

Will carry forward r=biesi; pinging Gerv for license approval
Attachment #656712 - Flags: review?(gerv)
Attachment #655519 - Attachment is obsolete: true
Attachment #655519 - Flags: review?(ted.mielczarek)
Attachment #655519 - Flags: review?(mcmanus)
Attachment #656759 - Flags: review?(ted.mielczarek)
Attachment #656759 - Flags: review?(cbiesinger)
Attachment #656712 - Attachment description: import of usrsctp library svn rev 8119 → import of usrsctp library svn rev 8119 r=biesi
Attachment #656179 - Flags: review?(mcmanus)
Comment on attachment 656759 [details] [diff] [review]
build libsctp within Mozilla tree

+++ b/netwerk/sctp/sctp_update.log
@@ -0,0 +1,8 @@
+sctp updated from CVS on Tue Mar 20 01:04:08 EDT 2012
+sctp updated from CVS on Fri Jun  1 01:31:43 EDT 2012
+sctp updated from CVS on Tue Jul 10 12:30:59 EDT 2012
+sctp updated from CVS on Thu Jul 12 00:31:32 EDT 2012
+sctp updated from CVS on Tue Jul 24 15:38:37 EDT 2012
+sctp updated from CVS on Sun Aug  5 03:51:50 EDT 2012
+sctp updated from CVS on Mon Aug  6 04:17:12 EDT 2012
+sctp updated to version 8119 from SVN on Wed Aug 29 22:06:23 EDT 2012

I'd just remove all the CVS lines; don't think the pre-hg import updates are relevant

+svn co --username your.username http://sctp-refimpl.googlecode.com/svn/trunk sctpSVN

fairly sure this one doesn't need a username
Attachment #656759 - Flags: review?(cbiesinger) → review+
Comment on attachment 656759 [details] [diff] [review]
build libsctp within Mozilla tree

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

Is this patch against mozilla-central? If so you'll need some bits from my patch on bug 782936. r- for that and a few other things.

::: configure.in
@@ +2934,5 @@
>  MOZ_CHECK_HEADERS(sys/quota.h sys/sysmacros.h)
> +MOZ_CHECK_HEADERS([linux/quota.h],,,[#include <sys/socket.h>])
> +
> +dnl SCTP support - needs various network include headers
> +MOZ_CHECK_HEADERS([linux/if_addr.h linux/rtnetlink.h],,,[#include <sys/socket.h>])

I'd just mash these two MOZ_CHECK_HEADERS lines together if they're both going to require sys/socket.h.

@@ +2941,5 @@
>  dnl NB - later gcc versions require -mmmx for this header to be successfully
>  dnl included (or another option which implies it, such as -march=pentium-mmx)
>  MOZ_CHECK_HEADERS(mmintrin.h)
>  
> +dnl Check for sin_len and sin6_len - used by SCTP; only appears in Mac/*BSD generally

This is basically the same diff hunk from bug 783843, but with sin6_len as well?

@@ +2944,5 @@
>  
> +dnl Check for sin_len and sin6_len - used by SCTP; only appears in Mac/*BSD generally
> +MOZ_CHECK_HEADERS(sys/types.h)
> +AC_MSG_CHECKING(for sockaddr_in.sin_len)
> +AC_CACHE_VAL(ac_cv_sockaddr_in_sin_len,

AC_CACHE_CHECK, as per the other patch.

::: netwerk/Makefile.in
@@ +25,5 @@
>    ipc  \
>    $(NULL)
>  
> +ifdef MOZ_SCTP
> +PARALLEL_DIRS += sctp

Just make this sctp/src and get rid of sctp/Makefile.in.

::: netwerk/sctp/Makefile.in
@@ +15,5 @@
> +  $(NULL)
> +
> +include $(topsrcdir)/config/rules.mk
> +
> +DEFINES += -DIMPL_NS_NET

This Makefile doesn't do anything useful. Get rid of it.

::: netwerk/sctp/src/Makefile.in
@@ +79,5 @@
> +ifeq ($(OS_TARGET),WINNT)
> +DEFINES += \
> +    -D__Userspace_os_Windows=1 \
> +    -D_LIB=1 \
> +    $(NULL)

Two-space indent.

@@ +80,5 @@
> +DEFINES += \
> +    -D__Userspace_os_Windows=1 \
> +    -D_LIB=1 \
> +    $(NULL)
> +CFLAGS += /I. /W3

I think you probably want to use -I. and -W3, but I'm not really sure why these are here anyway.

@@ +84,5 @@
> +CFLAGS += /I. /W3
> +else
> +ifeq ($(OS_TARGET),Darwin)
> +DEFINES += \
> +    -D__Userspace_os_Darwin=1 \

Here as well.

@@ +94,5 @@
> +DEFINES += -D__Userspace_os_Linux=1
> +else
> +ifeq ($(OS_TARGET),FreeBSD)
> +DEFINES += \
> +    -D__Userspace_os_FreeBSD=1 \

Two-space indent.

@@ +98,5 @@
> +    -D__Userspace_os_FreeBSD=1 \
> +    -U__FreeBSD__ \
> +    $(NULL)
> +else
> +#default_fallback; probably doesn't work

I'd prefer that this error out in configure on unsupported platforms then instead of people hitting weird build errors in the SCTP code.

::: netwerk/sctp/src/README.sctp
@@ +3,5 @@
> +The project is accessed via:
> +
> +svn co --username your.username https://sctp-refimpl.googlecode.com/svn/trunk sctpSVN
> +
> +If you are just wanting a read-only copy use

"If you just want a read-only copy use"

::: netwerk/sctp/update_sctp.sh
@@ +11,5 @@
> +#
> +# also assumes we've made *NO* changes to the SCTP sources!  If we do, we have to merge by
> +# hand after this process, or use a more complex one.
> +#
> +# For example, one could update an sctp library import head, and merge back to default.  Or keep a 

Extraneous tailing space here.
Attachment #656759 - Flags: review?(ted.mielczarek) → review-
merged in bug 783843 (which is r=ted)
Attachment #656759 - Attachment is obsolete: true
Attachment #659389 - Flags: review?(ted.mielczarek)
Attachment #656712 - Attachment is obsolete: true
Attachment #656712 - Flags: review?(gerv)
bug 787204 is the licensing (r+'d)
Depends on: 787204
Comment on attachment 656179 [details] [diff] [review]
Proposed upstream fixes to usrsctp library

Rev 8176 has merged these all in
Attachment #656179 - Attachment is obsolete: true
Attachment #659389 - Flags: review?(ted.mielczarek)
Should deal with all comments from Ted's review
Attachment #659389 - Attachment is obsolete: true
Attachment #659507 - Flags: review?(ted.mielczarek)
Comment on attachment 659507 [details] [diff] [review]
(and Bug 783843) build libsctp within Mozilla tree

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

r=me with comments addressed.

::: configure.in
@@ +2971,5 @@
> +                               [],
> +                               [ac_cv_sockaddr_in6_sin6_len=true],
> +                               [ac_cv_sockaddr_in6_sin6_len=false])])
> +if test "$ac_cv_sockaddr_in6_sin6_len" = true ; then
> +  AC_DEFINE(HAVE_SIN6_LEN)

Is it an error if only one of HAVE_SIN[6]_LEN is set? I'd assume only a seriously broken toolchain would hit that.

::: netwerk/sctp/src/Makefile.in
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DEPTH     = ../../..

DEPTH = @DEPTH@

@@ +23,5 @@
> +
> +EXPORTS_NAMESPACES = mozilla/net
> +
> +XPIDLSRCS = \
> +  $(NULL)

Why is this here? Just remove it.

@@ +25,5 @@
> +
> +XPIDLSRCS = \
> +  $(NULL)
> +
> +CSRCS = \

nit: here and elsewhere you should use := if you're assigning something that can be immediately evaluated.

@@ +58,5 @@
> +  usrsctp.h \
> +  $(NULL)
> +
> +LOCAL_INCLUDES = \
> +  -I$(srcdir)/../../base/src \

This would read better starting from $(topsrcdir), so it's clear where it's pointing.

@@ +97,5 @@
> +  -D__Userspace_os_FreeBSD=1 \
> +  -U__FreeBSD__ \
> +  $(NULL)
> +else
> +#error Unsupported platform!

#error is not a make thing. You want $(error). This is silly though, we should be testing platforms in configure, not here. I'd get rid of the nested ifeq/else chain entirely.

::: netwerk/sctp/src/README.sctp
@@ +8,5 @@
> +
> +svn co http://sctp-refimpl.googlecode.com/svn/trunk sctpSVN
> +
> +The usrsctp library is based on the FreeBSD kernel implementation, and the
> +userspace implementation was largely done my Michael Tuexen and Irene

typo: done by
Attachment #659507 - Flags: review?(ted.mielczarek) → review+
Blocks: 797084
Depends on: 798352
Blocks: 889136
You need to log in before you can comment on or make changes to this bug.