970550 - Bug 444328 broke the build on OpenBSD

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Landry Breuil (:gaston)]

Bug 444328 is likely the one breaking the build on OpenBSD (and probably other bsds) :

/var/buildslave-mozilla/mozilla-central-amd64/build/netwerk/base/src/nsSocketTransport2.cpp:2825:45: error: use of undeclared identifier 'TCP_KEEPIDLE'
    int err = setsockopt(sock, IPPROTO_TCP, TCP_KEEPIDLE,
                                            ^
/var/buildslave-mozilla/mozilla-central-amd64/build/netwerk/base/src/nsSocketTransport2.cpp:2834:41: error: use of undeclared identifier 'TCP_KEEPINTVL'
    err = setsockopt(sock, IPPROTO_TCP, TCP_KEEPINTVL,
                                        ^
/var/buildslave-mozilla/mozilla-central-amd64/build/netwerk/base/src/nsSocketTransport2.cpp:2842:41: error: use of undeclared identifier 'TCP_KEEPCNT'
    err = setsockopt(sock, IPPROTO_TCP, TCP_KEEPCNT,

(cf http://buildbot.rhaalovely.net/builders/mozilla-central-amd64/builds/1028/steps/build/logs/stdio)

OpenBSD only supports SO_KEEPALIVE setsockopt, see http://www.openbsd.org/cgi-bin/man.cgi?query=setsockopt&sektion=2

I'd suggest adding another #elif block similar to the macos block, using SO_KEEPALIVE instead of TCP_KEEPALIVE. I'll have to check what other bsds support..

Comment 1

[Landry Breuil (:gaston)]

According to http://fxr.watson.org/fxr/source/netinet/tcp.h#L166 freebsd has those TCP_KEEP* #defines

Comment 2

[Landry Breuil (:gaston)]

NetBSD and Dragonfly have TCP_KEEPCNT and TCP_KEEPINTVL but not TCP_KEEPALIVE ? Strange.. maybe that's TCP_KEEPIDLE instead

http://fxr.watson.org/fxr/source/netinet/tcp.h?v=DFBSD#L168

Comment 3

[Landry Breuil (:gaston)]

After digging more, netbsd, freebsd and dragonfly have TCP_KEEPCNT and TCP_KEEPINTVL documented in tcp(4) manpage.
http://www.manpages.info/freebsd/tcp.4.html

Comment 4

[Steve Workman [:sworkman] (INACTIVE)]

Hi Landry - thanks for filing the bug and for the investigation. It looks like there may be fragmented support on BSD. I'd suggest going with SO_KEEPALIVE only for now - that way the build will be up and running again. Or, if possible, use #ifdef TCP_KEEPCNT ... setsockopt(...TCP_KEEPCNT) ...#endif within an #elif BSD branch. The former is simpler, but we'd lose finer control of TCP keepalives on platforms that support it. The latter is more complex, and not so pretty.

Comment 5

[Jan Beich]

Why not combine?

  #ifdef XP_WIN
  // Windows
  #elif defined(TCP_KEEPALIVE)
  // OS X, QNX, Solaris, etc
  #elif defined(TCP_KEEPIDLE)
  // Linux and BSDs
  #elif defined(SO_KEEPALIVE)
  // fallback (OpenBSD here)
  #else
  // abort
  #endif

  // keep separate in case OS X implements them
  #ifdef TCP_KEEPCNT
  // Linux and BSDs
  #endif
  #ifdef TCP_KEEPINTVL
  // Linux and BSDs
  #endif

Comment 6

[Landry Breuil (:gaston)]

Attachment: Fix build on OpenBSD

Here's what i have that fixes the build for me, but i wouldnt mind going the way jan proposed - might be nicer.

Comment 7

[Landry Breuil (:gaston)]

Attachment: Fix build on OpenBSD, alternate #ifdef version

And here's the alternative version with #ifdefs.

Comment 8

[Landry Breuil (:gaston)]

Just for the record: OpenBSD has those *KEEP* options, but not available as options to setsockopt() (hence not defined like other bsds), only via userland sysctls:

$sysctl | grep keep                                                                                                  
net.inet.tcp.keepinittime=150
net.inet.tcp.keepidle=14400
net.inet.tcp.keepintvl=150
net.inet.tcp.always_keepalive=0

Comment 9

[Jan Beich]

Comment on attachment 8374404 [details] [diff] [review]
Fix build on OpenBSD

Why do you disable TCP_KEEPCNT and TCP_KEEPINTVL for other BSDs ? And I don't like having __WhateverBSD__ for the code that isn't strictly platform-specific.

There's also a typo in __DragonFly__ check.

Comment 10

[Jan Beich]

Comment on attachment 8374417 [details] [diff] [review]
Fix build on OpenBSD, alternate #ifdef version

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

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +2821,5 @@
>      }
>      return NS_OK;
>  
>  #elif defined(XP_UNIX)
> +#ifdef TCP_KEEPIDLE

After bug 969757 non-XP_WIN is a synonym for XP_UNIX. Checking for both is probably redundant. And we need to fallthrough to MOZ_ASSERT if both TCP_KEEPIDLE and SO_KEEPALIVE aren't available.

Comment 11

[Jan Beich]

Solaris may be interested in coverting XP_MACOSX to TCP_KEEPALIVE ifdef. It doesn't have TCP_KEEPIDLE, TCP_KEEPCNT and TCP_KEEPINTVL but is XP_UNIX like Darwin.

Comment 12

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8374417 [details] [diff] [review]
Fix build on OpenBSD, alternate #ifdef version

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

Thanks for the patch, Landry. Some changes, so r- for now. Also, this would need verification on the various tier 1 Linuxes, most importantly FxOS and Android, before moving forward.

Also, I have a minor concern that these preprocessor consts could be defined as vars in certain implementations, specifically Android kernel variants for FxOS. I think it's probably very unlikely, but it would be nice if we could have something like #if defined(ANDROID) || defined(TCP_KEEPIDLE). That way we're somewhat asserting support is available for keepalive config for FxOS. It's just a little bit of insurance baked into the code, just in case device manufacturers decide to play with the TCP implementation - I'd rather create an alert that the support was removed/changed than just assume. Like I wrote, it's a minor concern, but if we can bake it into the code, let's do it.

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +2821,5 @@
>      }
>      return NS_OK;
>  
>  #elif defined(XP_UNIX)
> +#ifdef TCP_KEEPIDLE

I'd rather keep the redundancy in here, just to be explicit. And my original idea for the MOZ_ASSERT was about platforms rather than sub-platform support. So, I wouldn't worry about that either.

@@ +2822,5 @@
>      return NS_OK;
>  
>  #elif defined(XP_UNIX)
> +#ifdef TCP_KEEPIDLE
> +    // Linux, FreeBSD & NetBSD uses sec; supports idle time, retry interval and ping count.

Please add a comment before #ifdef TCP_KEEPALIVE to explain that support for TCP keepalive config varies by UNIX/Linux platform, and is distinguishable by the existence of preprocessor constants.

80 chars per line max.

@@ +2834,5 @@
> +#elif defined(SO_KEEPALIVE)
> +    // OpenBSD uses sec; only supports idle time being set.
> +    int err = setsockopt(sock, IPPROTO_TCP, SO_KEEPALIVE,
> +                         &aIdleTime, sizeof(aIdleTime));
> +    if (NS_WARN_IF(err)) {

I don't think SO_KEEPALIVE is used in this way; AFAIK (please correct me), it is used to enable/disable TCP keepalives, not set the idle time. As such, it should be part of the NSPR call in SetKeepaliveEnabled. So, I think OpenBSD might not allow any configuration?? In the man page you provided it doesn't mention idle time. Let me know if you see something different.

Comment 13

[Landry Breuil (:gaston)]

Attachment: Fix build on OpenBSD, alternate #ifdef version

You're totally right, SO_KEEPALIVE isnt the same thing at setting the idle time via TCP_KEEPALIVE, so OpenBSD only supports system-wide settings via sysctls, and not per-socket settings via setsockopt().

That patch should address all the comments, i'm testing it here and also pushed it to try to ensure it doesnt break tier1.

https://tbpl.mozilla.org/?tree=Try&rev=f8810cd9c7bf

Comment 14

[Landry Breuil (:gaston)]

netwerk/base/src/nsSocketTransport2.cpp:2864:1: error: control may reach end of non-void function [-Werror,-Wreturn-type]

oops.. guess i need to put the last #endif _before_ return NS_OK if i want OpenBSD to reach the return..

Comment 15

[Jan Beich]

Attachment: Solaris fix on top of OpenBSD one

Darwin is both XP_MACOSX and XP_UNIX. Moving XP_UNIX ifdef before TCP_KEEPALIVE.

Comment 16

[Landry Breuil (:gaston)]

Attachment: Fix build, #ifdef alternate version v3

That one works on OpenBSD

Comment 17

[Jan Beich]

Comment on attachment 8375108 [details] [diff] [review]
Solaris fix on top of OpenBSD one

Nevermind, IllumOS seems to support TCP_KEEP(IDLE|INTVL|CNT).

Comment 18

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8375110 [details] [diff] [review]
Fix build, #ifdef alternate version v3

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

I'm going to give a tentative r+ to this. I actually like that it doesn't overlap with XP_MACOSX - we know what we expect there and I'd like to get errors if something is different, just like Android. But I'm open to reviewing a second patch if another platform is seriously broken here.

As I wrote before, some verifications that keepalive is still enabled on android would be good. A basic wireshark or tcpdump is good enough.

And, of course, no errors on try.

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +2820,5 @@
>      }
>      return NS_OK;
>  
>  #elif defined(XP_UNIX)
> +    // Not all *nix OSes support the following setsockopt() options

// ... but we assume they are supported in the Android kernel;
// build errors will tell us if they are not.

@@ +2823,5 @@
>  #elif defined(XP_UNIX)
> +    // Not all *nix OSes support the following setsockopt() options
> +#if defined(ANDROID) || defined(TCP_KEEPIDLE)
> +    // Linux, FreeBSD, DragonFly & NetBSD uses sec; supports idle time, retry
> +    // interval and ping count.

I think we can slim this comment down. We've already mentioned *nix variants, so let's just describe TCP_KEEPIDLE.
// Idle time until first keepalive probe; interval between ack'd probes; seconds.

@@ +2834,5 @@
>      }
>  
> +#endif
> +#if defined(ANDROID) || defined(TCP_KEEPINTVL)
> +    // They also have a few extra knobs to tweak

I should have removed this earlier ;)
// Interval between unack'd keepalive probes; seconds.

@@ +2849,1 @@
>      err = setsockopt(sock, IPPROTO_TCP, TCP_KEEPCNT,

// Number of unack'd keepalive probes before connection times out.

Comment 19

[Landry Breuil (:gaston)]

(In reply to Steve Workman [:sworkman] from comment #18)

> As I wrote before, some verifications that keepalive is still enabled on
> android would be good. A basic wireshark or tcpdump is good enough.

I'm definitely not an android hacker (nor plan to become one), but builds are available here:
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f8810cd9c7bf

Unless it's really blocking, i'll land this today with the comments fixed.

Comment 20

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d85a9041774

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5d85a9041774