Project

General

Profile

Actions

Feature #3128

closed

server.core-files support on FreeBSD proposal

Added by devnexen 9 months ago. Updated 9 months ago.

Status:
Fixed
Priority:
Low
Category:
core
Target version:
ASK QUESTIONS IN Forums:
No

Files

Actions #1

Updated by gstrauss 9 months ago

What commit or tag is your patch against? There seems to be some excess added lines which are already present in lighttpd git master branch.

Second, there should be a better solution than defining the internal __BSD_VISIBLE identifier for copy_file_range(), maybe #define _BSD_SOURCE 1 ?
FreeBSD 13 added copy_file_range()
https://cgit.freebsd.org/src/commit/?id=bbbbeca3e9a3
https://www.freebsd.org/cgi/man.cgi?query=copy_file_range&sektion=2&format=html

Actions #2

Updated by devnexen 9 months ago

Unfortunately the _BSD_SOURCE solution did not work out (I tried already anyway).
about the code source discrepancy, I used the github repo, is there any internal git maybe more advanced ? anyway the code is small if preferred it can be adapted most likely.

Actions #3

Updated by gstrauss 9 months ago

Yes, I can adapt your patch. Still, please verify local git repo from which you created your patch.
https://github.com/lighttpd/lighttpd1.4/blob/master/src/server.c contains 29 lines which your patch "adds".

The official lighttpd repo is git.lighttpd.net as documented in lighttpd source code and build instructions.
https://github.com/lighttpd/lighttpd1.4/ is a mirror.

Actions #4

Updated by devnexen 9 months ago

Here a new version of the patch w/o the useless changes.

Actions #5

Updated by gstrauss 9 months ago

Thanks for the updated patch. Have you tested the patches? (either the old or the new?)

#if defined(HAVE_SYS_PROCTL_H) && defined(PROC_TRACE_CTL_ENABLE)

I think that should be defined(HAVE_SYS_PROCCTL_H)

Actions #6

Updated by devnexen 9 months ago

ah; in a first iteration of the patch I set with only one `C` and forgot to correct afterwards and the previous update squashed the include, good catch.

Actions #7

Updated by gstrauss 9 months ago

I see that you added #include <sys/procctl.h>. Good. That was missing in earlier patches, too.

Actions #8

Updated by gstrauss 9 months ago

Your comment about DragonFlyBSD is unclear.
I think you meant to say more directly -- instead of to imply -- that DragonFlyBSD does not have PROC_TRACE_CTL_ENABLE
https://gitweb.dragonflybsd.org/dragonfly.git/blob/HEAD:/sys/sys/procctl.h

I also fixed the patch for SCons to detect sys/procctl.h

On my dev branch:
https://git.lighttpd.net/lighttpd/lighttpd1.4/commit/f36908de33c60f6cca3a6ae8be625d8f25e1c7ac

[Edit: I should mention that my patch is untested. Please test in your environment.]

Actions #9

Updated by gstrauss 9 months ago

  • Category set to core
  • Status changed from New to Patch Pending
  • Target version changed from 1.4.xx to 1.4.64
Actions #10

Updated by devnexen 9 months ago

All good except the copy_file_range fix being not present, I got the compile warning.
Indeed for dragonfly I meant it did not support it.

Actions #11

Updated by gstrauss 9 months ago

All good except the copy_file_range fix being not present

Not forgotten, but not related to this issue: "server.core-files support on FreeBSD proposal".
Separate items will be in separate commits. The copy_file_range() is of limited use in lighttpd mod_webdav since it is used only with
webdav.opts += ("deprecated-unsafe-partial-put" => enable). I would like to find a better #define to enable the feature and avoid the compiler warning.

Actions #12

Updated by gstrauss 9 months ago

https://github.com/freebsd/freebsd-src/blob/main/sys/sys/cdefs.h#L740
If _C99_SOURCE is the issue for you due to build config, it can be undef'd.

I am considering the following patch to limit the change to FreeBSD 13, in which copy_file_range() was introduced.

--- a/src/mod_webdav.c
+++ b/src/mod_webdav.c
@@ -169,6 +169,13 @@
 #ifndef _GNU_SOURCE
 #define _GNU_SOURCE
 #endif
+#ifdef HAVE_COPY_FILE_RANGE
+#ifdef __FreeBSD__ /* FreeBSD strict compliance hides libc extensions */
+#undef _ANSI_SOURCE
+#undef _C99_SOURCE
+#undef _C11_SOURCE
+#endif
+#endif

 #include "first.h"      /* first */
 #include "sys-mmap.h" 

... However, the above might break in the future if the build system switches to -std=c11. On the other hand, #define __BSD_VISIBLE 1 might conflict if some other config triggers #define __BSD_VISIBLE 0 in sys/cdefs.h

Actions #13

Updated by gstrauss 9 months ago

This is probably more future-proof:

--- a/src/mod_webdav.c
+++ b/src/mod_webdav.c
@@ -169,6 +169,13 @@
 #ifndef _GNU_SOURCE
 #define _GNU_SOURCE
 #endif
+#ifdef HAVE_COPY_FILE_RANGE
+#ifdef __FreeBSD__ /* FreeBSD strict compliance hides libc extensions */
+#define _XOPEN_SOURCE 700
+#define _ISOC11_SOURCE
+#define __BSD_VISIBLE 1
+#endif
+#endif

 #include "first.h"      /* first */
 #include "sys-mmap.h" 

On my dev branch: https://git.lighttpd.net/lighttpd/lighttpd1.4/commit/9001903c1c65330cd9fab4f9c1699c4ffdc09e82

Actions #14

Updated by devnexen 9 months ago

well the second version, _XOPEN_SOURCE is already defined. but fine otherwise.

Actions #15

Updated by gstrauss 9 months ago

  • Status changed from Patch Pending to Fixed
Actions #16

Updated by devnexen 9 months ago

After a full clean compile I get the error finally.

So what I gather :
- HAVE_COPY_FILE_RANGE can t be defined where it is since it comes from config.h.
- Once removed, furthermore _ISOC11_SOURCE causes build to fails.
- Thus

#ifdef __FreeBSD__
#define __BSD_VISIBLE 1
#endif

seems working.

Actions #17

Updated by devnexen 9 months ago

Alternatively, if you really want to keep on HAVE_COPY_FILE_RANGE, you can replace by something along those lines

#if defined(__FreeBSD__)
#include <sys/param.h>
#if __FreeBSD_version >= 1300000
#define __BSD_VISIBLE 1
#endif
#endif

Actions #18

Updated by gstrauss 9 months ago

well the second version, _XOPEN_SOURCE is already defined. but fine otherwise.

I don't think you and I understand the above "but fine otherwise" to mean the same thing. I'm not interested in your opinions, but whether or not you tested on FreeBSD 13, which I do not have.

Before you responded, you clearly did not test very well (or at all) the patches you submitted or the patches I suggested trying.

Actions #19

Updated by gstrauss 9 months ago

  • Status changed from Fixed to Reopened
  • Priority changed from Normal to Low
Actions #20

Updated by devnexen 9 months ago

Fair point.

So I reclone your repo and. branch and with

#ifdef __FreeBSD__
#define __BSD_VISIBLE 1
#endif

the build passes totally.

Actions #21

Updated by devnexen 9 months ago

Sorry again, I will be more careful next time.

Actions #22

Updated by gstrauss 9 months ago

The code in mod_webdav which uses copy_file_range() is wrapped in #ifdef HAVE_COPY_FILE_RANGE.
HAVE_COPY_FILE_RANGE is defined in config.h, if detected during the build configuration process.
If the build configuration process is not adjusted to detect copy_file_range(), then HAVE_COPY_FILE_RANGE will not get defined, and copy_file_range() will not be used in mod_webdav.

I expect the build to compile with or without

#ifdef __FreeBSD__
#define __BSD_VISIBLE 1
#endif

Which build tools are you using with lighttpd? autoconf, meson, cmake, or scons?

Actions #23

Updated by devnexen 9 months ago

Yes true, the copy_file_range code use it s wrapped in this way. I use cmake.

So with the actual code I get

[ 62%] Building C object build/CMakeFiles/mod_webdav.dir/mod_webdav.c.o
/home/dcarlier/Contribs/lighttpd1.4/src/mod_webdav.c:4564:18: warning: implicit declaration of function 'copy_file_range' is invalid in C99 [-Wimplicit-function-declaration]
            wr = copy_file_range(c->file.fd,&zoff,fd,&ooff,(size_t)cqlen, 0);
                 ^
1 warning generated.
[ 62%] Linking C shared library mod_webdav.so

now the code looks like this

/* linkat() fstatat() unlinkat() fdopendir() NAME_MAX */
#if !defined(_XOPEN_SOURCE) || _XOPEN_SOURCE-0 < 700
#undef  _XOPEN_SOURCE
#define _XOPEN_SOURCE 700
/* NetBSD dirent.h improperly hides fdopendir() (POSIX.1-2008) declaration
 * which should be visible with _XOPEN_SOURCE 700 or _POSIX_C_SOURCE 200809L */
#ifdef __NetBSD__
#define _NETBSD_SOURCE
#endif
#endif
/* DT_UNKNOWN DTTOIF() */
#ifndef _GNU_SOURCE
#define _GNU_SOURCE
#endif
#ifdef HAVE_COPY_FILE_RANGE /* FreeBSD 13+ */
#ifdef __FreeBSD__ /* FreeBSD strict compliance hides libc extensions */
/*#define _XOPEN_SOURCE 700*//*(defined above)*/
#define _ISOC11_SOURCE
#define __BSD_VISIBLE 1
#endif
#endif

#include "first.h"      /* first */

thus the first header included occurs after this.
So If I remove HAVE_COPY_FILE_RANGE I get

Consolidate compiler generated dependencies of target mod_webdav
[ 62%] Building C object build/CMakeFiles/mod_webdav.dir/mod_webdav.c.o
In file included from /home/dcarlier/Contribs/lighttpd1.4/src/mod_webdav.c:178:
In file included from /home/dcarlier/Contribs/lighttpd1.4/src/first.h:45:
In file included from /usr/include/sys/types.h:43:
/usr/include/sys/cdefs.h:723:20: error: invalid token at start of a preprocessor expression
#if _ISOC11_SOURCE || (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L)
Actions #24

Updated by devnexen 9 months ago

If I remove the _ISOC11_SOURCE definition it passes

Consolidate compiler generated dependencies of target mod_webdav
[ 62%] Building C object build/CMakeFiles/mod_webdav.dir/mod_webdav.c.o
[ 62%] Linking C shared library mod_webdav.so
[ 62%] Built target mod_webdav
[ 63%] Built target mod_wstunnel
[ 70%] Built target test_configfile
[ 91%] Built target test_mod
[ 98%] Built target test_common
[ 99%] Built target fcgi-responder
[100%] Built target scgi-responder
Actions #25

Updated by gstrauss 9 months ago

Please try the change to +#define _ISOC11_SOURCE 1 below.

--- a/src/mod_webdav.c
+++ b/src/mod_webdav.c
@@ -169,6 +169,13 @@
 #ifndef _GNU_SOURCE
 #define _GNU_SOURCE
 #endif
+#ifdef HAVE_COPY_FILE_RANGE
+#ifdef __FreeBSD__ /* FreeBSD strict compliance hides libc extensions */
+/*#define _XOPEN_SOURCE 700*//*(defined above)*/
+#define _ISOC11_SOURCE 1
+#define __BSD_VISIBLE 1
+#endif
+#endif

 #include "first.h"      /* first */
 #include "sys-mmap.h" 

Actions #26

Updated by gstrauss 9 months ago

Bug in my patch: #ifdef HAVE_COPY_FILE_RANGE /* FreeBSD 13+ */ will not be true before #include "first.h", so I need to remove that, and to test on earlier versions of FreeBSD, but I think the patch will be ok without that initial #ifdef HAVE_COPY_FILE_RANGE

Actions #27

Updated by devnexen 9 months ago

So with your suggestion still same issues mentioned above unfortunately, this time I show you what the compiler/linker uses :

[ 62%] Building C object build/CMakeFiles/mod_webdav.dir/mod_webdav.c.o
cd /home/dcarlier/Contribs/lighttpd1.4/build/build && /usr/bin/cc -DHAVE_CONFIG_H -D_DEFAULT_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGE_FILES -D_TIME_BITS=64 -Dmod_webdav_EXPORTS -I/home/dcarlier/Contribs/lighttpd1.4/build/build -I/home/dcarlier/Contribs/lighttpd1.4/src -I/usr/local/include -std=gnu99 -pipe -Wall -g -Wshadow -W -pedantic  -fPIC -MD -MT build/CMakeFiles/mod_webdav.dir/mod_webdav.c.o -MF CMakeFiles/mod_webdav.dir/mod_webdav.c.o.d -o CMakeFiles/mod_webdav.dir/mod_webdav.c.o -c /home/dcarlier/Contribs/lighttpd1.4/src/mod_webdav.c
/home/dcarlier/Contribs/lighttpd1.4/src/mod_webdav.c:4564:18: warning: implicit declaration of function 'copy_file_range' is invalid in C99 [-Wimplicit-function-declaration]
            wr = copy_file_range(c->file.fd,&zoff,fd,&ooff,(size_t)cqlen, 0);
                 ^
1 warning generated.
[ 62%] Linking C shared library mod_webdav.so

If I remove HAVE_COPY_FILE_RANGE

[ 62%] Building C object build/CMakeFiles/mod_webdav.dir/mod_webdav.c.o
cd /home/dcarlier/Contribs/lighttpd1.4/build/build && /usr/bin/cc -DHAVE_CONFIG_H -D_DEFAULT_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGE_FILES -D_TIME_BITS=64 -Dmod_webdav_EXPORTS -I/home/dcarlier/Contribs/lighttpd1.4/build/build -I/home/dcarlier/Contribs/lighttpd1.4/src -I/usr/local/include -std=gnu99 -pipe -Wall -g -Wshadow -W -pedantic  -fPIC -MD -MT build/CMakeFiles/mod_webdav.dir/mod_webdav.c.o -MF CMakeFiles/mod_webdav.dir/mod_webdav.c.o.d -o CMakeFiles/mod_webdav.dir/mod_webdav.c.o -c /home/dcarlier/Contribs/lighttpd1.4/src/mod_webdav.c
In file included from /home/dcarlier/Contribs/lighttpd1.4/src/mod_webdav.c:178:
In file included from /home/dcarlier/Contribs/lighttpd1.4/src/first.h:45:
In file included from /usr/include/sys/types.h:43:
/usr/include/sys/cdefs.h:723:20: error: invalid token at start of a preprocessor expression
#if _ISOC11_SOURCE || (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L)
                   ^
1 error generated.
gmake[2]: *** [build/CMakeFiles/mod_webdav.dir/build.make:76: build/CMakeFiles/mod_webdav.dir/mod_webdav.c.o] Error 1
Actions #28

Updated by devnexen 9 months ago

I wrote in same time than your last comment so we agree on a point at least.

Actions #29

Updated by gstrauss 9 months ago

Are you testing with this patch?

--- a/src/mod_webdav.c
+++ b/src/mod_webdav.c
@@ -169,6 +169,11 @@
 #ifndef _GNU_SOURCE
 #define _GNU_SOURCE
 #endif
+#ifdef __FreeBSD__ /* FreeBSD strict compliance hides libc extensions */
+/*#define _XOPEN_SOURCE 700*//*(defined above)*/
+#define _ISOC11_SOURCE 1
+#define __BSD_VISIBLE 1
+#endif

 #include "first.h"      /* first */
 #include "sys-mmap.h" 

Actions #30

Updated by devnexen 9 months ago

this last patch still triggers the same issue with _ISOC11_SOURCE definition

[ 62%] Building C object build/CMakeFiles/mod_webdav.dir/mod_webdav.c.o
cd /home/dcarlier/Contribs/lighttpd1.4/build/build && /usr/bin/cc -DHAVE_CONFIG_H -D_DEFAULT_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGE_FILES -D_TIME_BITS=64 -Dmod_webdav_EXPORTS -I/home/dcarlier/Contribs/lighttpd1.4/build/build -I/home/dcarlier/Contribs/lighttpd1.4/src -I/usr/local/include -std=gnu99 -pipe -Wall -g -Wshadow -W -pedantic  -fPIC -MD -MT build/CMakeFiles/mod_webdav.dir/mod_webdav.c.o -MF CMakeFiles/mod_webdav.dir/mod_webdav.c.o.d -o CMakeFiles/mod_webdav.dir/mod_webdav.c.o -c /home/dcarlier/Contribs/lighttpd1.4/src/mod_webdav.c
In file included from /home/dcarlier/Contribs/lighttpd1.4/src/mod_webdav.c:178:
In file included from /home/dcarlier/Contribs/lighttpd1.4/src/first.h:45:
In file included from /usr/include/sys/types.h:43:
/usr/include/sys/cdefs.h:723:20: error: invalid token at start of a preprocessor expression
#if _ISOC11_SOURCE || (defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L)
                   ^
1 error generated.
gmake[2]: *** [build/CMakeFiles/mod_webdav.dir/build.make:76: build/CMakeFiles/mod_webdav.dir/mod_webdav.c.o] Error 1
Actions #31

Updated by devnexen 9 months ago

hold on, it passes. :-)

Actions #32

Updated by devnexen 9 months ago

/* linkat() fstatat() unlinkat() fdopendir() NAME_MAX */
#if !defined(_XOPEN_SOURCE) || _XOPEN_SOURCE-0 < 700
#undef  _XOPEN_SOURCE
#define _XOPEN_SOURCE 700
/* NetBSD dirent.h improperly hides fdopendir() (POSIX.1-2008) declaration
 * which should be visible with _XOPEN_SOURCE 700 or _POSIX_C_SOURCE 200809L */
#ifdef __NetBSD__
#define _NETBSD_SOURCE
#endif
#endif
/* DT_UNKNOWN DTTOIF() */
#ifndef _GNU_SOURCE
#define _GNU_SOURCE
#endif
#ifdef __FreeBSD__ /* FreeBSD strict compliance hides libc extensions */
/*#define _XOPEN_SOURCE 700*//*(defined above)*/
#define _ISOC11_SOURCE 1
#define __BSD_VISIBLE 1
#endif

#include "first.h"      /* first */
#include "sys-mmap.h" 
#include <sys/types.h>

should be the version committed IMHO

Actions #33

Updated by gstrauss 9 months ago

  • Status changed from Reopened to Fixed

That is what I posted 8 comments ago, which took you more than a few tries to properly add to your build and compile.

Actions

Also available in: Atom