Hello!

On Thu, Jan 17, 2019 at 06:18:29PM +0300, Sergey Kandaurov wrote:

>
> > On 17 Jan 2019, at 16:22, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > Hello!
> >
> > On Thu, Jan 17, 2019 at 01:29:56PM +0300, Sergey Kandaurov wrote:
> >
> >>
> >>
> >>> On 17 Jan 2019, at 08:43, Sepherosa Ziehau <sepherosa@gmail.com> wrote:
> >>>
> >>> What's the preferred way to handle this? I am not sure whether you
> >>> guys allow __FreeBSD_version testing etc.
> >>>
> >>
> >> This could be solved with autotests.
> >
> > As long as we only care about different FreeBSD versions, this
> > might as well be an auto/os/freebsd test based on $version, or
> > just a define based on __FreeBSD_version in
> > src/os/unix/ngx_freebsd_config.h.
> >
> >> # HG changeset patch
> >> # User Sergey Kandaurov <pluknet@nginx.com>
> >> # Date 1547720890 0
> >> # Thu Jan 17 10:28:10 2019 +0000
> >> # Node ID d28513cd71bce227b4e159b7a3f518aa504232f0
> >> # Parent 6d15e452fa2eaf19408e24a0d0fcc3a31344a289
> >> Fixed portability issues with union sigval.
> >>
> >> The sival_ptr field is now preferably used.
> >>
> >> diff --git a/auto/unix b/auto/unix
> >> --- a/auto/unix
> >> +++ b/auto/unix
> >> @@ -523,6 +523,7 @@ if [ $NGX_FILE_AIO = YES ]; then
> >> ngx_feature_libs=
> >> ngx_feature_test="struct aiocb iocb;
> >> iocb.aio_sigevent.sigev_notify = SIGEV_KEVENT;
> >> + iocb.aio_sigevent.sigev_value.sival_ptr = NULL;
> >> (void) aio_read(&iocb)"
> >> . auto/feature
> >>
> >> @@ -532,6 +533,22 @@ if [ $NGX_FILE_AIO = YES ]; then
> >>
> >> if [ $ngx_found = no ]; then
> >>
> >> + ngx_feature="kqueue AIO support (legacy)"
> >> + ngx_feature_name="NGX_HAVE_FILE_AIO_LEGACY"
> >> + ngx_feature_test="struct aiocb iocb;
> >> + iocb.aio_sigevent.sigev_notify = SIGEV_KEVENT;
> >> + iocb.aio_sigevent.sigev_value.sigval_ptr = NULL;
> >> + (void) aio_read(&iocb)"
> >> + . auto/feature
> >
> > Note that this will test for "kqueue AIO support (legacy)" on each
> > Linux build with aio.
>
> That is unfortunate.
>
> >> +
> >> + if [ $ngx_found = yes ]; then
> >> + CORE_SRCS="$CORE_SRCS $FILE_AIO_SRCS"
> >> + have=NGX_HAVE_FILE_AIO . auto/have
> >> + fi
> >> + fi
> >> +
> >> + if [ $ngx_found = no ]; then
> >> +
> >> ngx_feature="Linux AIO support"
> >> ngx_feature_name="NGX_HAVE_FILE_AIO"
> >> ngx_feature_run=no
> >> diff --git a/src/os/unix/ngx_file_aio_read.c b/src/os/unix/ngx_file_aio_read.c
> >> --- a/src/os/unix/ngx_file_aio_read.c
> >> +++ b/src/os/unix/ngx_file_aio_read.c
> >> @@ -110,8 +110,12 @@ ngx_file_aio_read(ngx_file_t *file, u_ch
> >> #if (NGX_HAVE_KQUEUE)
> >> aio->aiocb.aio_sigevent.sigev_notify_kqueue = ngx_kqueue;
> >> aio->aiocb.aio_sigevent.sigev_notify = SIGEV_KEVENT;
> >> +#if !(NGX_HAVE_FILE_AIO_LEGACY)
> >> + aio->aiocb.aio_sigevent.sigev_value.sival_ptr = ev;
> >> +#else
> >> aio->aiocb.aio_sigevent.sigev_value.sigval_ptr = ev;
> >> #endif
> >> +#endif
> >> ev->handler = ngx_file_aio_event_handler;
> >>
> >> n = aio_read(&aio->aiocb);
> >
> > A simplier solution might be to always use sival_ptr, and define
> > it to sigval_ptr on old FreeBSDs.
>
> This looks simpler indeed.
>
> > Alternatively, we can consider dropping file AIO support for these
> > old FreeBSD versions. This shouldn't be a big deal as this is an
> > optional feature which is not enabled by default.
>
> And that's what I'd prefer for --test-build-eventport workaround.
>
> What about these patches?
> For DragonFly this should effectively match initial submission.
>
> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1547734251 0
> # Thu Jan 17 14:10:51 2019 +0000
> # Node ID baab2b35e8cc79cfdf9924d1752348e97c1da13e
> # Parent 6d15e452fa2eaf19408e24a0d0fcc3a31344a289
> Fixed portability issues with union sigval.

This needs to be extended with some background information -
notably why it used to be sigval_ptr, and why to switch to
sival_ptr.

>
> diff --git a/src/os/unix/ngx_file_aio_read.c b/src/os/unix/ngx_file_aio_read.c
> --- a/src/os/unix/ngx_file_aio_read.c
> +++ b/src/os/unix/ngx_file_aio_read.c
> @@ -110,7 +110,7 @@ ngx_file_aio_read(ngx_file_t *file, u_ch
> #if (NGX_HAVE_KQUEUE)
> aio->aiocb.aio_sigevent.sigev_notify_kqueue = ngx_kqueue;
> aio->aiocb.aio_sigevent.sigev_notify = SIGEV_KEVENT;
> - aio->aiocb.aio_sigevent.sigev_value.sigval_ptr = ev;
> + aio->aiocb.aio_sigevent.sigev_value.sival_ptr = ev;
> #endif
> ev->handler = ngx_file_aio_event_handler;
>
> diff --git a/src/os/unix/ngx_freebsd_config.h b/src/os/unix/ngx_freebsd_config.h
> --- a/src/os/unix/ngx_freebsd_config.h
> +++ b/src/os/unix/ngx_freebsd_config.h
> @@ -91,6 +91,10 @@
> #if (NGX_HAVE_FILE_AIO)
> #include <aio.h>
> typedef struct aiocb ngx_aiocb_t;
> +
> +#if (__FreeBSD_version < 700005 && !defined __DragonFly__)
> +#define sival_ptr sigval_ptr
> +#endif
> #endif
>
>

Minor style nits:

diff --git a/src/os/unix/ngx_freebsd_config.h b/src/os/unix/ngx_freebsd_config.h
--- a/src/os/unix/ngx_freebsd_config.h
+++ b/src/os/unix/ngx_freebsd_config.h
@@ -89,12 +89,14 @@


#if (NGX_HAVE_FILE_AIO)
+
#include <aio.h>
typedef struct aiocb ngx_aiocb_t;

#if (__FreeBSD_version < 700005 && !defined __DragonFly__)
-#define sival_ptr sigval_ptr
+#define sival_ptr sigval_ptr
#endif
+
#endif


> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1547736673 0
> # Thu Jan 17 14:51:13 2019 +0000
> # Node ID c66911fc9924a60bb5d691ca00bc2fb1c3032866
> # Parent baab2b35e8cc79cfdf9924d1752348e97c1da13e
> Removed --test-build-eventport workaround for old FreeBSD versions.
>
> diff --git a/src/event/modules/ngx_eventport_module.c b/src/event/modules/ngx_eventport_module.c
> --- a/src/event/modules/ngx_eventport_module.c
> +++ b/src/event/modules/ngx_eventport_module.c
> @@ -250,9 +250,7 @@ ngx_eventport_init(ngx_cycle_t *cycle, n
>
> ngx_memzero(&sev, sizeof(struct sigevent));
> sev.sigev_notify = SIGEV_PORT;
> -#if !(NGX_TEST_BUILD_EVENTPORT)
> sev.sigev_value.sival_ptr = &pn;
> -#endif
>
> if (timer_create(CLOCK_REALTIME, &sev, &event_timer) == -1) {
> ngx_log_error(NGX_LOG_EMERG, cycle->log, ngx_errno,

Looks fine.

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel