Closed Bug 844430 Opened 7 years ago Closed 6 years ago

fix libsctp on OpenBSD/NetBSD/Dragonfly

Categories

(Core :: Networking, defect)

Other
OpenBSD
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: gaston, Assigned: gaston)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch Fix sctp on openbsd (obsolete) — Splinter Review
Similar to #790343 & needed for the work-in-progress webrtc-on-bsd (#807492), as of now if i enable webrtc on openbsd it fails badly in netwerk/sctp. The attached wip patch allows me to build under that dir fine.
Added all the build DEFINES goo for other bsds while here.

I'm pretty sure it needs to be refined (for openbsd and tested on netbsd ?) and of course pushed upstream.

I need to include our sys/uio.h otherwise struct iovec is not defined, but our uio_seg enum doesnt have UIO_NOCOPY, hence the hackish #define.
Blocks: 807492
I suspect DragonFly target is wrong, see bug 788414.
Attachment #717484 - Attachment is patch: true
cc'ing upstream for feedback (and yes, DragonFly target is wrong, agreed - but then to be perfect it'd have to be tested there and on NetBSD too :)
Upstream would prefer not to use Userspace_os_BSD at all. Just introduce and use
__Userspace_os_NetBSD, __Userspace_os_OpenBSD, and __Userspace_os_DragonFlyBSD.

This make some #ifs more complex, but allows a simple migration to a better #ifdefs
usage I have in mind for the future.

Best regards
Michael
Attached patch Fix libsctp on OpenBSD (obsolete) — Splinter Review
Ok, so here's the minimal bare patch that allows me to build sctp on OpenBSD :

- added simpler boilerplate for NetBSD/DragonFly, they'll need to fill in the blanks
- added a comment about UIO_NOCOPY explaining why it's needed.

I only have 3 warnings left :

/src/mozilla-central/netwerk/sctp/src/user_socket.c:3120:16: warning: incompatible pointer types passing 'long *' to parameter of type 'const time_t *' (aka 'const int *') [-Wincompatible-pointer-types]
        t = localtime(&tv.tv_sec);
                      ^~~~~~~~~~
/usr/include/time.h:124:36: note: passing argument to parameter here
struct tm *localtime(const time_t *);
                                   ^
/src/mozilla-central/netwerk/sctp/src/user_socket.c:3123:45: warning: format specifies type 'int' but the argument has type 'long' [-Wformat]
                 t->tm_hour, t->tm_min, t->tm_sec, tv.tv_usec);
                                                   ^~~~~~~~~~

/src/mozilla-central/netwerk/sctp/src/user_socket.c:720:8: warning: case value not in enumerated type 'enum uio_seg' [-Wswitch]
gmake[6]: Leaving directory `/usr/obj/m-c/netwerk/locales'
                case UIO_NOCOPY:
                     ^
/src/mozilla-central/netwerk/sctp/src/user_socketvar.h:80:20: note: expanded from macro 'UIO_NOCOPY'
#define UIO_NOCOPY 2

The last one is unavoidable imo..
Assignee: nobody → landry
Attachment #717484 - Attachment is obsolete: true
Attachment #718636 - Flags: review?(rjesup)
Attachment #718636 - Flags: feedback?(tuexen)
(In reply to Landry Breuil (:gaston) from comment #4)
> Created attachment 718636 [details] [diff] [review]
> Fix libsctp on OpenBSD
Thanks for the patch. I have applied them upstream. See
http://code.google.com/p/sctp-refimpl/source/detail?r=8425
(With on exception: The change affecting an Android and Linux change
doesn't contain the Android and Linux change, since I don't understand
why the Linux part is necessary).
> 
> Ok, so here's the minimal bare patch that allows me to build sctp on OpenBSD
> :
> 
> - added simpler boilerplate for NetBSD/DragonFly, they'll need to fill in
> the blanks
> - added a comment about UIO_NOCOPY explaining why it's needed.
> 
> I only have 3 warnings left :
> 
> /src/mozilla-central/netwerk/sctp/src/user_socket.c:3120:16: warning:
> incompatible pointer types passing 'long *' to parameter of type 'const
> time_t *' (aka 'const int *') [-Wincompatible-pointer-types]
>         t = localtime(&tv.tv_sec);
Fixed in
http://code.google.com/p/sctp-refimpl/source/detail?r=8426
>                       ^~~~~~~~~~
> /usr/include/time.h:124:36: note: passing argument to parameter here
> struct tm *localtime(const time_t *);
>                                    ^
> /src/mozilla-central/netwerk/sctp/src/user_socket.c:3123:45: warning: format
> specifies type 'int' but the argument has type 'long' [-Wformat]
>                  t->tm_hour, t->tm_min, t->tm_sec, tv.tv_usec);
>                                                    ^~~~~~~~~~
Already fixed upstream.
> 
> /src/mozilla-central/netwerk/sctp/src/user_socket.c:720:8: warning: case
> value not in enumerated type 'enum uio_seg' [-Wswitch]
> gmake[6]: Leaving directory `/usr/obj/m-c/netwerk/locales'
>                 case UIO_NOCOPY:
>                      ^
> /src/mozilla-central/netwerk/sctp/src/user_socketvar.h:80:20: note: expanded
> from macro 'UIO_NOCOPY'
> #define UIO_NOCOPY 2
> 
> The last one is unavoidable imo..
I took the stuff out:
http://code.google.com/p/sctp-refimpl/source/detail?r=8424

Thanks a lot for the patches.

Best regards
Michael
(In reply to Michael Tüxen from comment #5)
> (In reply to Landry Breuil (:gaston) from comment #4)
> > Created attachment 718636 [details] [diff] [review]
> > Fix libsctp on OpenBSD
> Thanks for the patch. I have applied them upstream. See
> http://code.google.com/p/sctp-refimpl/source/detail?r=8425
> (With on exception: The change affecting an Android and Linux change
> doesn't contain the Android and Linux change, since I don't understand
> why the Linux part is necessary).

Ah, the include <pthread.h> in sctp_os_userspace.h for Linux/Android ? apparently it was added in http://hg.mozilla.org/mozilla-central/rev/3253c4a1f213.

> Thanks a lot for the patches.

Many thanks for applying them!

Randall, i suppose that now for m-c it'd be simpler to file a new bug to update the sctp copy from upstream svn, or backport just the commits to m-c in that bug and they'll be overwritten by the next update ?
Squashed patch of the 3 upstream revs, in case we want to push them as-is without updating from svn right now.
Attachment #718636 - Attachment is obsolete: true
Attachment #718636 - Flags: review?(rjesup)
Attachment #718636 - Flags: feedback?(tuexen)
Attachment #719359 - Flags: review?(rjesup)
Randell: Up to you. But there are several fixes upstream and also a support for binding the same port as requested.
See https://bugzilla.mozilla.org/show_bug.cgi?id=841126

Best regards
Michael
jesup: review ping ? Or should i file a new bug to update sctp ?
A request for updating the SCTP sources has already been filed.
See https://bugzilla.mozilla.org/show_bug.cgi?id=841126

Updating to the current version will also fix:
https://bugzilla.mozilla.org/show_bug.cgi?id=846012
https://bugzilla.mozilla.org/show_bug.cgi?id=845513
https://bugzilla.mozilla.org/show_bug.cgi?id=845518

Best regards
Michael
Now that bug 855620 merged those upstream revs, i think we only need the Makefile.in chunk to fix sctp on the other BSDs.
Attachment #719359 - Attachment is obsolete: true
Attachment #719359 - Flags: review?(rjesup)
Attachment #756755 - Flags: review?(rjesup)
Comment on attachment 756755 [details] [diff] [review]
Fix libsctp on OpenBSD/NetBSD/Dragonfly

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

Ok for now; they're going to update SCTP's OS config system, but not yet
Attachment #756755 - Flags: review?(rjesup) → review+
Summary: fix libsctp on OpenBSD → fix libsctp on OpenBSD/NetBSD/Dragonfly
https://hg.mozilla.org/mozilla-central/rev/308462ba17ba
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.