844430 - fix libsctp on OpenBSD/NetBSD/Dragonfly

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Landry Breuil (:gaston)]

Attachment: Fix sctp on openbsd

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.

Comment 1

[Jan Beich]

I suspect DragonFly target is wrong, see bug 788414.

Comment 2

[Landry Breuil (:gaston)]

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 :)

Comment 3

[Michael Tüxen]

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

Comment 4

[Landry Breuil (:gaston)]

Attachment: Fix libsctp on OpenBSD

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..

Comment 5

[Michael Tüxen]

(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

Comment 6

[Landry Breuil (:gaston)]

(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 ?

Comment 7

[Landry Breuil (:gaston)]

Attachment: Fix libsctp on OpenBSD (upstream r8424->8426)

Squashed patch of the 3 upstream revs, in case we want to push them as-is without updating from svn right now.

Comment 8

[Michael Tüxen]

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

Comment 9

[Landry Breuil (:gaston)]

jesup: review ping ? Or should i file a new bug to update sctp ?

Comment 10

[Michael Tüxen]

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

Comment 11

[Landry Breuil (:gaston)]

Attachment: Fix libsctp on OpenBSD/NetBSD/Dragonfly

Now that bug 855620 merged those upstream revs, i think we only need the Makefile.in chunk to fix sctp on the other BSDs.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 13

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/308462ba17ba

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/308462ba17ba