kqueue build fixes #777

pluknet · 2025-07-08T20:43:19Z

No description provided.

The kevent udata field is special in that we maintain compatibility with NetBSD versions that predate using the "void *" type. The fix is to cast to intermediate uintptr_t that is casted back to "void *" where appropriate.

bavshin-f5 · 2025-07-08T21:25:25Z

auto/unix



-if [ "$NGX_SYSTEM" = "NetBSD" ]; then
+if [ "$NGX_SYSTEM" = "NetBSD" -a ${NGX_RELEASE%%.*} -lt 10 ]; then


Would be more accurate to check for NetBSD/src@560337f, the first version change after NetBSD/src@9425a92

#if (defined __NetBSD_Version__ && __NetBSD_Version__ < 999001600)

I wonder if EV_SET could make this and all the casts unnecessary. I assume we weren't using EV_SET initially because it did not exist between FreeBSD 4.1 and 4.3, but that doesn't seem to be an issue today.

Would be more accurate to check for NetBSD/src@560337f, the first version change after NetBSD/src@9425a92

#if (defined __NetBSD_Version__ && __NetBSD_Version__ < 999001600)

It might be, though I don't think we need.

The XX.99.YY version pattern reflects an interim development version, as documented in "sys/param.h":

* m = minor version; a minor number of 99 indicates current.

We don't care about them in more popular OSes, let alone NetBSD.

Note also that kevent change wasn't reflected in __NetBSD_Version__ bump, so why should we care.

I wonder if EV_SET could make this and all the casts unnecessary. I assume we weren't using EV_SET initially because it did not exist between FreeBSD 4.1 and 4.3, but that doesn't seem to be an issue today.

I don't think it's a good reason to break FreeBSD 4.x.
Currently, nginx should be buildable down to FreeBSD 2.2.9 as relatively recently fixed in 5fd9cfa.

It might be, though I don't think we need.

I missed the step where I suggest to move this check to ngx_kqueue_module.c. It was already odd that we have #if defined(__NetBSD__) in shell, and got even more awkward with the version check.

I don't think it's a good reason to break FreeBSD 4.x.

>= 4.1,< 4.3. Given that 4.x branch ended with 4.11.0 in 2005, it might be reasonable to assume that nobody runs FreeBSD 4.2 today. But let's leave the discussion about the legacy systems support for another time.

See also #777 (comment)

arut · 2025-07-09T17:10:54Z

The first patch (warning) looks ok.

arut · 2025-07-09T17:19:25Z

Using NGX_RELEASE to test available features seems like something we've never done before and it looks like a workaround. Wouldn't it be better to write a normal feature test?

arut · 2025-07-09T17:32:13Z

Using NGX_RELEASE to test available features seems like something we've never done before and it looks like a workaround. Wouldn't it be better to write a normal feature test?

Maybe something like this (untested)

diff --git a/auto/unix b/auto/unix
index 0dd66cfcd..24dff3360 100644
--- a/auto/unix
+++ b/auto/unix
@@ -129,9 +129,25 @@ if test -z "$NGX_KQUEUE_CHECKED"; then
 fi
 
 
+ngx_found=no
+
 if [ "$NGX_SYSTEM" = "NetBSD" ]; then
 
-    # NetBSD 2.0 incompatibly defines kevent.udata as "intptr_t"
+    # NetBSD 2.0 up to 10.0 incompatibly defines kevent.udata as "intptr_t"
+
+    ngx_feature="kqueue's udata"
+    ngx_feature_name="NGX_KQUEUE_UDATA"
+    ngx_feature_run=no
+    ngx_feature_incs="#include <sys/event.h>"
+    ngx_feature_path=
+    ngx_feature_libs=
+    ngx_feature_test="struct kevent  kev1, kev2;
+                      (void) (kev1.udata + kev2.udata)"
+
+    . auto/feature
+fi
+
+if [ $ngx_found = yes ]; then
 
     cat << END >> $NGX_AUTO_CONFIG_H

pluknet · 2025-07-09T18:40:29Z

Using NGX_RELEASE to test available features seems like something we've never done before and it looks like a workaround. Wouldn't it be better to write a normal feature test?

For the first, I don't take why testing a prior knowledge.
Then, I consider this an API bug (once they fixed it), not a feature, hence it doesn't belong there.

Shell parameter expansion fits perfectly here.
It has more compact notation and efficient to running a feature test.

Further, I suspect that writing such a feature test can be a challenge.
While it should be clearly written, it should also not be a compiler test.
I don't think it worth the effort.

Concerning the "we've never done before", see how we test OpenSSL and certain OS versions in sources.
It's the same but moved to config time.
Also we have FreeBSD version test in autotests, but I consider it ugly and fragile.

As for the latter, I think it actually needs to be moved out from autotests (see also somewhat related in #777 (comment)).

A cumulative patch against master:

diff --git a/auto/unix b/auto/unix
index 0dd66cfcd..6087e04fd 100644
--- a/auto/unix
+++ b/auto/unix
@@ -129,26 +129,6 @@ if test -z "$NGX_KQUEUE_CHECKED"; then
 fi
 
 
-if [ "$NGX_SYSTEM" = "NetBSD" ]; then
-
-    # NetBSD 2.0 incompatibly defines kevent.udata as "intptr_t"
-
-    cat << END >> $NGX_AUTO_CONFIG_H
-
-#define NGX_KQUEUE_UDATA_T
-
-END
-
-else
-    cat << END >> $NGX_AUTO_CONFIG_H
-
-#define NGX_KQUEUE_UDATA_T  (void *)
-
-END
-
-fi
-
-
 ngx_feature="crypt()"
 ngx_feature_name="NGX_HAVE_CRYPT"
 ngx_feature_run=no
diff --git a/src/event/modules/ngx_kqueue_module.c b/src/event/modules/ngx_kqueue_module.c
index 9c7244c45..f070de284 100644
--- a/src/event/modules/ngx_kqueue_module.c
+++ b/src/event/modules/ngx_kqueue_module.c
@@ -10,6 +10,15 @@
 #include <ngx_event.h>
 
 
+/* NetBSD 2.0 up to 10.0 incompatibly defines kevent.udata as "intptr_t" */
+
+#if (defined __NetBSD__ && __NetBSD_Version__ < 1000000000)
+#define NGX_KQUEUE_UDATA_T
+#else
+#define NGX_KQUEUE_UDATA_T  (void *)
+#endif
+
+
 typedef struct {
     ngx_uint_t  changes;
     ngx_uint_t  events;
@@ -191,7 +200,7 @@ ngx_kqueue_init(ngx_cycle_t *cycle, ngx_msec_t timer)
         kev.flags = EV_ADD|EV_ENABLE;
         kev.fflags = 0;
         kev.data = timer;
-        kev.udata = 0;
+        kev.udata = NGX_KQUEUE_UDATA_T (uintptr_t) 0;
 
         ts.tv_sec = 0;
         ts.tv_nsec = 0;
@@ -237,7 +246,7 @@ ngx_kqueue_notify_init(ngx_log_t *log)
     notify_kev.data = 0;
     notify_kev.flags = EV_ADD|EV_CLEAR;
     notify_kev.fflags = 0;
-    notify_kev.udata = 0;
+    notify_kev.udata = NGX_KQUEUE_UDATA_T (uintptr_t) 0;
 
     if (kevent(ngx_kqueue, &notify_kev, 1, NULL, 0, NULL) == -1) {
         ngx_log_error(NGX_LOG_ALERT, log, ngx_errno,

ac000 · 2025-07-09T18:42:10Z

For the record here's the NetBSD
commit
that made the udata change.

pluknet · 2025-07-10T13:03:45Z

A cumulative patch against master:

[...]

Updated patch series.

arut · 2025-07-10T16:17:09Z

No functional changes.

I'd argue changing nginx API (NGX_KQUEUE_UDATA_T is no longer defined publicly) is a functional change.

arut · 2025-07-10T16:19:17Z

... -Wint-conversion compiler errors ...

Technically, it's -Werror=int-conversion:

src/event/modules/ngx_kqueue_module.c:253:22: error: assignment to 'void *' from 'long unsigned int'
makes pointer from integer without a cast [-Werror=int-conversion]

arut

Otherwise looks ok.

The NGX_KQUEUE_UDATA_T macro is used to compensate the incompatible kqueue() API in NetBSD, it doesn't really belong to feature tests. The change limits the macro visibility to the kqueue event module. Moving from autotests also simplifies testing a particular NetBSD version as seen in a subsequent change.

pluknet · 2025-07-11T11:45:43Z

Applied, thanks.

The kevent udata field was changed from intptr_t to "void *", similar to other BSDs and Darwin. The NGX_KQUEUE_UDATA_T macro is adjusted to reflect that change, fixing -Werror=int-conversion errors.

pluknet · 2025-07-11T12:25:41Z

Applied some more cosmetic polishing to commit logs.

pluknet requested a review from arut July 8, 2025 20:43

pluknet self-assigned this Jul 8, 2025

pluknet added the bug label Jul 8, 2025

bavshin-f5 reviewed Jul 8, 2025

View reviewed changes

pluknet force-pushed the gcc-null-warning branch from 4bd9452 to 2eaf81a Compare July 10, 2025 13:02

arut approved these changes Jul 10, 2025

View reviewed changes

pluknet force-pushed the gcc-null-warning branch from 2eaf81a to 903ed33 Compare July 11, 2025 11:45

Events: compatibility with NetBSD 10.0 in kqueue.

3f4f0a2

The kevent udata field was changed from intptr_t to "void *", similar to other BSDs and Darwin. The NGX_KQUEUE_UDATA_T macro is adjusted to reflect that change, fixing -Werror=int-conversion errors.

pluknet force-pushed the gcc-null-warning branch from 903ed33 to 3f4f0a2 Compare July 11, 2025 12:25

pluknet merged commit c52c569 into nginx:master Jul 11, 2025
1 check passed

pluknet deleted the gcc-null-warning branch July 11, 2025 12:25

pluknet added this to the nginx-1.29.1 milestone Jul 11, 2025



		if [ "$NGX_SYSTEM" = "NetBSD" ]; then
		if [ "$NGX_SYSTEM" = "NetBSD" -a ${NGX_RELEASE%%.*} -lt 10 ]; then

kqueue build fixes #777

kqueue build fixes #777

Uh oh!

Conversation

pluknet commented Jul 8, 2025

Uh oh!

bavshin-f5 Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

pluknet Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

bavshin-f5 Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

pluknet Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

arut commented Jul 9, 2025

Uh oh!

arut commented Jul 9, 2025

Uh oh!

arut commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pluknet commented Jul 9, 2025

Uh oh!

ac000 commented Jul 9, 2025

Uh oh!

pluknet commented Jul 10, 2025

Uh oh!

arut commented Jul 10, 2025

Uh oh!

arut commented Jul 10, 2025

Uh oh!

arut left a comment

Choose a reason for hiding this comment

Uh oh!

pluknet commented Jul 11, 2025

Uh oh!

pluknet commented Jul 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

arut commented Jul 9, 2025 •

edited

Loading