Bug 11816 - samba 4.4.0 fails to compile on illumos/Solaris due to FIONREAD not being declared
Summary: samba 4.4.0 fails to compile on illumos/Solaris due to FIONREAD not being dec...
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Build (show other bugs)
Version: 4.4.0
Hardware: All Solaris
: P5 trivial (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 11873
  Show dependency treegraph
 
Reported: 2016-03-30 14:43 UTC by Jorge Schrauwen
Modified: 2021-02-11 14:17 UTC (History)
5 users (show)

See Also:


Attachments
patch for the copy in source3 (321 bytes, patch)
2016-03-30 14:45 UTC, Jorge Schrauwen
no flags Details
patch for the copy in source4 (313 bytes, patch)
2016-03-30 14:46 UTC, Jorge Schrauwen
no flags Details
2nd version of the patch for source3 (825 bytes, patch)
2016-03-31 14:38 UTC, Jorge Schrauwen
no flags Details
2nd version of the patch for source4 (962 bytes, patch)
2016-03-31 14:39 UTC, Jorge Schrauwen
no flags Details
lib/nss_wrapper configure script fix for SOLARIS_GETGRENT_R (841 bytes, patch)
2016-03-31 14:40 UTC, Jorge Schrauwen
no flags Details
Patch (2.15 KB, patch)
2016-04-04 06:19 UTC, Volker Lendecke
no flags Details
Patches for v4-4-test (3.79 KB, patch)
2016-04-27 17:28 UTC, Stefan Metzmacher
vl: review+
Details
Patches for v4-3-test (3.79 KB, patch)
2016-04-27 17:29 UTC, Stefan Metzmacher
vl: review+
Details
Patches for v4-2-test (3.79 KB, patch)
2016-04-27 17:33 UTC, Stefan Metzmacher
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jorge Schrauwen 2016-03-30 14:43:02 UTC
I'm building Samba 4.4.0 on illumos (OpenSolaris). Including just sys/inotify.h is not sufficient on illumos/Solaris (should be the same for the now Oracle Solaris).

Adding 2 ifdef's to include sys/filio.h on SunOS makes it compile.

Simple patch below, not sure if they are proper as I just do packaging.


$NetBSD$

--- source3/smbd/notify_inotify.c.orig  2016-01-26 11:45:46.000000000 +0000
+++ source3/smbd/notify_inotify.c
@@ -27,6 +27,9 @@
 #include "lib/util/sys_rw_data.h"
 
 #include <sys/inotify.h>
+#ifdef __sun
+#include <sys/filio.h>
+#endif
 
 /* glibc < 2.5 headers don't have these defines */
 #ifndef IN_ONLYDIR
$NetBSD$

--- source4/ntvfs/sysdep/inotify.c.orig 2016-01-26 11:45:46.000000000 +0000
+++ source4/ntvfs/sysdep/inotify.c
@@ -30,6 +30,9 @@
 #include "param/param.h"
 
 #include <sys/inotify.h>
+#ifdef __sun
+#include <sys/filio.h>
+#endif
 
 /* glibc < 2.5 headers don't have these defines */
 #ifndef IN_ONLYDIR
Comment 1 Jorge Schrauwen 2016-03-30 14:45:56 UTC
Created attachment 11950 [details]
patch for the copy in source3
Comment 2 Jorge Schrauwen 2016-03-30 14:46:18 UTC
Created attachment 11951 [details]
patch for the copy in source4
Comment 3 Jorge Schrauwen 2016-03-30 14:47:17 UTC
Also added both patches as a .patch file.

So far Samba 4.4 seems to compile much smoother than 4.3 (I dropped all my patches aside these 2)
Comment 4 Volker Lendecke 2016-03-31 07:10:43 UTC
(In reply to Jorge Schrauwen from comment #1)
> patch for the copy in source3

We don't ususally do the #ifdef <some-platform> anymore. It should be possible to write a configure test that checks whether inotify can be included on its own.

As a separate question -- does Illumos really support inotify?
Comment 5 Jorge Schrauwen 2016-03-31 07:19:00 UTC
I double checked:
https://github.com/joyent/illumos-joyent/blob/master/usr/src/lib/libc/port/sys/inotify.c

Exists, but it looks like it did not make it in all distro's yet. (Down side of being fragmented I guess).

Probably why configure detects inotify support and tries to enable it.
So if inotify.h is found and we are SunOS we need sys/filio.h, else well there is no problem because inotify.h is not found anwyay.


Can you point me in the right direction for the configuration test? I it's something relatively trivial I might be able to get a patch for that instread.
Comment 6 Volker Lendecke 2016-03-31 10:38:12 UTC
(In reply to Jorge Schrauwen from comment #5)
> Can you point me in the right direction for the configuration test? I it's
> something relatively trivial I might be able to get a patch for that
> instread.

I'd just do a conf.CHECK_HEADERS('sys/filio.h') and if that exists #include it in the two inotify.c files with a comment that Illumos needs it. Assuming Illumos can't be fixed :-)
Comment 7 Jorge Schrauwen 2016-03-31 11:47:12 UTC
(In reply to Volker Lendecke from comment #6)
> <jperkin> quite a lot of stuff assumes inotify == Linux, which is why we deliberately break inotify.h in pkgsrc builds to avoid it being picked up

Seems the inotify on illumos is not fully the same as on Linux.
It may be better to ignore inotify.h on SunOS in general then.
Comment 8 Volker Lendecke 2016-03-31 11:53:40 UTC
(In reply to Jorge Schrauwen from comment #7)
> Seems the inotify on illumos is not fully the same as on Linux.
> It may be better to ignore inotify.h on SunOS in general then.

Ok, in that case you might avoid the check for inotify in lib/replace/wscript on Illumos. In wscripts it is okay to have platform specific stuff, for example search for "aix" in that file.
Comment 9 Jorge Schrauwen 2016-03-31 11:57:47 UTC
More comments from jperkin that may be of interest:
> <jperkin> the linux man page _does_ specify sys/ioctl.h which would be enough on illumos to pull in filio.h, so if samba is using ioctl(2) but not including sys/ioctl.h then it's more likely a samba thing (and perhaps they haven't noticed because some other header on linux is pulling in sys/ioctl.h behind the scenes)
Comment 10 Jorge Schrauwen 2016-03-31 12:00:28 UTC
(In reply to Jorge Schrauwen from comment #9)
<jperkin> sorry, that's not accurate, it's hidden behind -DBSD_COMP
<jperkin> I dunno, as I said need a standards wonk ;)
Comment 11 Jorge Schrauwen 2016-03-31 14:38:57 UTC
Created attachment 11954 [details]
2nd version of the patch for source3

2nd version updates the configure script to skip detection of inotify if the os is Solaris (Oracle Solaris does not implement it, illumos sometimes does but it is not a drop in replacement for the linux ones)

This one is for the script in source3
Comment 12 Jorge Schrauwen 2016-03-31 14:39:20 UTC
Created attachment 11955 [details]
2nd version of the patch for source4

2nd version updates the configure script to skip detection of inotify if the os is Solaris (Oracle Solaris does not implement it, illumos sometimes does but it is not a drop in replacement for the linux ones)

This one is for the script in source4
Comment 13 Jorge Schrauwen 2016-03-31 14:40:36 UTC
Created attachment 11956 [details]
lib/nss_wrapper configure script fix for SOLARIS_GETGRENT_R

Found another small error but this time in the script for lib/nss
Comment 14 Volker Lendecke 2016-04-04 06:19:24 UTC
Created attachment 11963 [details]
Patch

Does this patch also do it I've slightly changed the source4 piece.
Comment 15 Jorge Schrauwen 2016-04-04 08:09:25 UTC
Builds fine with the following applied:
11963
11956
Comment 16 Jorge Schrauwen 2016-04-13 06:42:04 UTC
Patches apply fine against 4.4.2 also.
Comment 17 Jorge Schrauwen 2016-04-23 10:20:05 UTC
Now that badlock has passed, anything holding these back from getting included?
Comment 18 Stefan Metzmacher 2016-04-27 17:28:42 UTC
Created attachment 12027 [details]
Patches for v4-4-test
Comment 19 Stefan Metzmacher 2016-04-27 17:29:18 UTC
Created attachment 12028 [details]
Patches for v4-3-test
Comment 20 Stefan Metzmacher 2016-04-27 17:33:18 UTC
Created attachment 12029 [details]
Patches for v4-2-test
Comment 21 Jorge Schrauwen 2016-04-27 18:33:28 UTC
(In reply to Stefan Metzmacher from comment #18)
Just tested this version of the patch, looks good to me.
Comment 22 Karolin Seeger 2016-04-28 11:02:06 UTC
Pushed to autobuild-v4-[4|3|2]-test.
Comment 23 Karolin Seeger 2016-04-29 09:00:51 UTC
(In reply to Karolin Seeger from comment #22)
Pushed to all branches.
Closing out bug report.

Thanks!
Comment 24 YOUZHONG YANG 2016-05-27 19:09:40 UTC
illumos does not deliver inotify.h, SmartOS does.

FIONREAD is defined in /usr/include/sys/filio.h. We've worked around the compiling issue and enabled inotify on SmartOS for at least two years, it's a nice feature enabling Samba to do change notification, otherwise you have to use other plugin for change notification.

Simply disabling inotify is not really a good solution, IMHO.
Comment 25 Jorge Schrauwen 2016-05-27 19:14:04 UTC
(In reply to YOUZHONG YANG from comment #24)
Are you sure it is working correctly when inotify support is compiled in?

Comment #7, seems to indicate that Joyent themselves do not build with it in pkgsrc tree to avoid issues.
Comment 26 YOUZHONG YANG 2016-05-27 19:17:33 UTC
(In reply to Jorge Schrauwen from comment #25)

We've used it in production environment for long time, yes, I'm pretty sure it works.
Comment 27 Jorge Schrauwen 2016-05-27 21:13:02 UTC
Not so very sure how the build system works but I wipped up a patch.
I test build is crunshing but it takes a few hours.

$NetBSD$

--- source3/wscript.orig        2016-05-02 07:29:51.000000000 +0000
+++ source3/wscript
@@ -142,6 +142,12 @@ long ret = splice(0,0,1,0,400,SPLICE_F_M
         conf.CHECK_HEADERS('sys/inotify.h')
         if "HAVE_SYS_INOTIFY_H" in conf.env:
            conf.DEFINE('HAVE_INOTIFY', 1)
+    else:
+        conf.CHECK_HEADERS('sys/inotify.h', add_headers=False)
+        conf.CHECK_HEADERS('sys/filio.h', add_headers=False)
+        if (conf.CONFIG_SET('HAVE_SYS_INOTIFY_H') and conf.CONFIG_SET('HAVE_SYS_FILIO_H')):
+           conf.DEFINE('HAVE_LINUX_INOTIFY', 1)
+           conf.DEFINE('BSD_COMP', 1)

     # Check for kernel change notify support
     conf.CHECK_CODE('''
$NetBSD$

--- source4/ntvfs/sysdep/wscript_configure.orig 2016-05-02 07:29:51.000000000 +0000
+++ source4/ntvfs/sysdep/wscript_configure
@@ -9,6 +9,13 @@ if host_os.rfind('sunos') == -1:
     conf.CHECK_HEADERS('sys/inotify.h', add_headers=False)
     if (conf.CONFIG_SET('HAVE_SYS_INOTIFY_H')):
         conf.DEFINE('HAVE_LINUX_INOTIFY', 1)
+else:
+    conf.CHECK_HEADERS('sys/inotify.h', add_headers=False)
+    conf.CHECK_HEADERS('sys/filio.h', add_headers=False)
+    if (conf.CONFIG_SET('HAVE_SYS_INOTIFY_H') and conf.CONFIG_SET('HAVE_SYS_FILIO_H')):
+        conf.DEFINE('HAVE_LINUX_INOTIFY', 1)
+        conf.DEFINE('BSD_COMP', 1)
+

 conf.CHECK_DECLS('F_SETLEASE', headers='linux/fcntl.h', reverse=True)
 conf.CHECK_DECLS('SA_SIGINFO', headers='signal.h', reverse=True)
Comment 28 Jorge Schrauwen 2016-05-27 22:33:32 UTC
(In reply to Jorge Schrauwen from comment #27)
Well that one failed, I'll look more at it tomorrow.
Comment 29 Jorge Schrauwen 2016-05-28 10:48:55 UTC
(In reply to YOUZHONG YANG from comment #26)
I added a new set of patches here as bug 11939

With these it compiles fine again. I have no real test case for the inotify though.