[PATCHES v4] Another round of FreeBSD developer build fixes

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCHES v4] Another round of FreeBSD developer build fixes

Samba - samba-technical mailing list
Hi,

After Andrew committed some of the FreeBSD build fixes, here's another
round of reduced patch set. It includes:

- Patches which were not explicitly nacked but did not land yet
- Reworked pam_wrapper patches, already upstream and reviewed by Andreas
- Reworked pragma-less handling of readline.h issues, according to tip
by Timur Bakeyev

It does not include parts of the original patch set which still require
rework:
- the sysacls patch (v3 1/38) which Andrew felt was smelly - after
closer examination I think it would have broken other
non-Linux/nop-FreeBSD builds.
- remaining #pragma-bearing patchs in pam_winbind (v3 13/38 and 14/38)

Review appreciated,
Uri.

fix-freebsd-developer-build-v4.patch.txt (35K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v4] Another round of FreeBSD developer build fixes

Samba - samba-technical mailing list
On Fri, 2017-11-24 at 00:58 +0200, Uri Simchoni via samba-technical
wrote:

> Hi,
>
> After Andrew committed some of the FreeBSD build fixes, here's another
> round of reduced patch set. It includes:
>
> - Patches which were not explicitly nacked but did not land yet
> - Reworked pam_wrapper patches, already upstream and reviewed by Andreas
> - Reworked pragma-less handling of readline.h issues, according to tip
> by Timur Bakeyev
>
> It does not include parts of the original patch set which still require
> rework:
> - the sysacls patch (v3 1/38) which Andrew felt was smelly - after
> closer examination I think it would have broken other
> non-Linux/nop-FreeBSD builds.
> - remaining #pragma-bearing patchs in pam_winbind (v3 13/38 and 14/38)
>
> Review appreciated,
> Uri.

I'll push some more of these.  I'm just putting off trying to
understand the _TYPE_MINIMUM change and I don't really like

[PATCH v4 01/13] s4-cli: increment once in the for loop

I would like some broader comment on:

[PATCH v4 08/13] build: remove -Wcast-align from developer build

I've skipped this one:
[PATCH v4 09/13] tevent: mark some functions as private
until I can work out what _PRIVATE_ actually does.

Another task in this area is this pull request by Sonic (CC'ed):

https://github.com/samba-team/samba/pull/105

If someone could have a look at this and make sense of what it that
would be most welcome.

Andrew Bartlett

--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba





Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v4] Another round of FreeBSD developer build fixes

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi Uri,

with the vfs_full_audit patch in that patchset I now get these build
failures:

../source3/modules/vfs_full_audit.c: In function ‘smb_full_audit_set_quota’:
../source3/modules/vfs_full_audit.c:722:54: error: zero-length
gnu_printf format string [-Werror=format-zero-length]
  do_log(SMB_VFS_OP_SET_QUOTA, (result >= 0), handle, "");
...

when building with --picky-developer on fedora27.

Could you have a look?

Thanks,
Guenther


On 23/11/17 23:58, Uri Simchoni via samba-technical wrote:

> Hi,
>
> After Andrew committed some of the FreeBSD build fixes, here's another
> round of reduced patch set. It includes:
>
> - Patches which were not explicitly nacked but did not land yet
> - Reworked pam_wrapper patches, already upstream and reviewed by Andreas
> - Reworked pragma-less handling of readline.h issues, according to tip
> by Timur Bakeyev
>
> It does not include parts of the original patch set which still require
> rework:
> - the sysacls patch (v3 1/38) which Andrew felt was smelly - after
> closer examination I think it would have broken other
> non-Linux/nop-FreeBSD builds.
> - remaining #pragma-bearing patchs in pam_winbind (v3 13/38 and 14/38)
>
> Review appreciated,
> Uri.
>

--
Günther Deschner                    GPG-ID: 8EE11688
Red Hat                         [hidden email]
Samba Team                              [hidden email]


signature.asc (223 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v4] Another round of FreeBSD developer build fixes

Samba - samba-technical mailing list
On 11/24/2017 05:37 PM, Günther Deschner wrote:

> Hi Uri,
>
> with the vfs_full_audit patch in that patchset I now get these build
> failures:
>
> ../source3/modules/vfs_full_audit.c: In function ‘smb_full_audit_set_quota’:
> ../source3/modules/vfs_full_audit.c:722:54: error: zero-length
> gnu_printf format string [-Werror=format-zero-length]
>   do_log(SMB_VFS_OP_SET_QUOTA, (result >= 0), handle, "");
> ...
>
> when building with --picky-developer on fedora27.
>
> Could you have a look?
>
> Thanks,
> Guenther
>
Strange... Samba master builds just fine on my fedora27 (gcc 7.2.1). We
added a -Wno-format-zero-length to the cflags (see commit
e973ac06431183b25cecf61053f3a23c91b88de6) and I can see that it's part
of the build.

I used:
./configure.developer --with-selftest-prefix=./bin/ab --picky-developer
--with-profiling-data --prefix=/home/vagrant/samba/bin/samba/prefix

And also with the --with-system-mitkrb5 variant.

Can you see if you have this flag on your build?

Thanks,
Uri.

>
> On 23/11/17 23:58, Uri Simchoni via samba-technical wrote:
>> Hi,
>>
>> After Andrew committed some of the FreeBSD build fixes, here's another
>> round of reduced patch set. It includes:
>>
>> - Patches which were not explicitly nacked but did not land yet
>> - Reworked pam_wrapper patches, already upstream and reviewed by Andreas
>> - Reworked pragma-less handling of readline.h issues, according to tip
>> by Timur Bakeyev
>>
>> It does not include parts of the original patch set which still require
>> rework:
>> - the sysacls patch (v3 1/38) which Andrew felt was smelly - after
>> closer examination I think it would have broken other
>> non-Linux/nop-FreeBSD builds.
>> - remaining #pragma-bearing patchs in pam_winbind (v3 13/38 and 14/38)
>>
>> Review appreciated,
>> Uri.
>>
>
>


signature.asc (484 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v4] Another round of FreeBSD developer build fixes

Samba - samba-technical mailing list
Oh right, I must had forgotten to reconfigure in between, now it works
like a charm.

Sorry for the noise :-)

Cheers,
Guenther

On 24/11/17 19:32, Uri Simchoni wrote:

> On 11/24/2017 05:37 PM, Günther Deschner wrote:
>> Hi Uri,
>>
>> with the vfs_full_audit patch in that patchset I now get these build
>> failures:
>>
>> ../source3/modules/vfs_full_audit.c: In function ‘smb_full_audit_set_quota’:
>> ../source3/modules/vfs_full_audit.c:722:54: error: zero-length
>> gnu_printf format string [-Werror=format-zero-length]
>>   do_log(SMB_VFS_OP_SET_QUOTA, (result >= 0), handle, "");
>> ...
>>
>> when building with --picky-developer on fedora27.
>>
>> Could you have a look?
>>
>> Thanks,
>> Guenther
>>
>
> Strange... Samba master builds just fine on my fedora27 (gcc 7.2.1). We
> added a -Wno-format-zero-length to the cflags (see commit
> e973ac06431183b25cecf61053f3a23c91b88de6) and I can see that it's part
> of the build.
>
> I used:
> ./configure.developer --with-selftest-prefix=./bin/ab --picky-developer
> --with-profiling-data --prefix=/home/vagrant/samba/bin/samba/prefix
>
> And also with the --with-system-mitkrb5 variant.
>
> Can you see if you have this flag on your build?
>
> Thanks,
> Uri.
>
>>
>> On 23/11/17 23:58, Uri Simchoni via samba-technical wrote:
>>> Hi,
>>>
>>> After Andrew committed some of the FreeBSD build fixes, here's another
>>> round of reduced patch set. It includes:
>>>
>>> - Patches which were not explicitly nacked but did not land yet
>>> - Reworked pam_wrapper patches, already upstream and reviewed by Andreas
>>> - Reworked pragma-less handling of readline.h issues, according to tip
>>> by Timur Bakeyev
>>>
>>> It does not include parts of the original patch set which still require
>>> rework:
>>> - the sysacls patch (v3 1/38) which Andrew felt was smelly - after
>>> closer examination I think it would have broken other
>>> non-Linux/nop-FreeBSD builds.
>>> - remaining #pragma-bearing patchs in pam_winbind (v3 13/38 and 14/38)
>>>
>>> Review appreciated,
>>> Uri.
>>>
>>
>>
>
>

--
Günther Deschner                    GPG-ID: 8EE11688
Red Hat                         [hidden email]
Samba Team                              [hidden email]


signature.asc (223 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v4] Another round of FreeBSD developer build fixes

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On 11/24/2017 02:09 AM, Andrew Bartlett via samba-technical wrote:
> I don't really like
>
> [PATCH v4 01/13] s4-cli: increment once in the for loop

What do you have in mind? I agree with the compile that incrementing
both in the for-loop and inside the loop is not very good - people
looking at the code might miss the increment. Shall we transform it into
a while loop?

> I would like some broader comment on:
>
> [PATCH v4 08/13] build: remove -Wcast-align from developer build
>
That commit message was already considerably longer than the average
commit message. Following is a broader commit message, please tell if
this is closer to what you had in mind.

    build: remove -Wcast-align from developer build

    -Wcast-align is a warning that warns against casting to a pointer
    with greater alignment. Prior to this patch it was enabled in
    picky developer builds, and this patch disables it.

    On some CPU architectures (e.g. ARM, MIPS), a load/store instruction
    is required to be from an address whose alignment matches the
    load/store size, e.g. a 32-bit load has to access a 32-bit aligned
    address.

    Compilers generally emit code that is guaranteed to comply with this
    requirement. As part of keeping that guarantee, each data type has
    an alignment associated with it, where a variable of that type is
    placed at an address which is a multiple of that alignment.

    Now, suppose a pointer to type A, with alignment of 2, is cast to a
    pointer to B, whose alignment is 4. Suddenly we lose the guarantee
    that our pointer is properly aligned, which can cause the program to
    crash on some architectures. The -Wcast-align warning warns against
    this.

    That would suggest that -Wcast-align is a good idea.

    However, casting to a pointer of greater alignment is not
    necessarily a bug. The pointer might be known to possess the desired
    alignment. For example:
    1. The original pointer got its value by casting from a greater
       alignment. That is, we cast B* to A* and back to B*. An example
       is casting from sockaddr * to sockaddr_in * - we know the
       sockaddr pointer really points to a sockaddr_in or a
       sockaddr_storage.

    2. A pointer is known to point to a member of a B structure
       with greater alignment, then calculating the address of the B*
       from the A* is legal, although it involves a cast from A* to
       B* (this BTW has a workaround of casting to void * somewhere
       along the way - see container_of macro in the Linux kernel).

    The Samba code base has some correct code that triggers the
    -Wcast-align warning if enabled. The reason we don't get it all the
    time is that we usually run developer builds with GCC and on
    x86/x86_64 architectures. Those architectures don't have alignment
    requirements, and GCC chooses not to emit a warning in this case.
    That makes the -Wcast-align a no-op on Samba's autobuild.

    OTOH, clang does emit warnings (turned to errors in developer
    build).

    So, this warning buys us nothing with x86_64+GCC, and breaks the
    build with clang (and probably on ARM+GCC in developer mode).

    In order to enable this warning properly, we need to map the valid
    uses and disable it there (e.g. there are some C files that
    manipulate sockets, disable the warning on those files. Also have a
    container_of macro that doesn't trigger the warning. Until this is
    done, in order to allow building developer build with clang, this
    warning is not enabled.

    Signed-off-by: Uri Simchoni <[hidden email]>

Thanks,
Uri.

Reply | Threaded
Open this post in threaded view
|

-Wcast-qual Re: [PATCHES v4] Another round of FreeBSD developer build fixes

Samba - samba-technical mailing list
On Sat, 2017-11-25 at 00:37 +0200, Uri Simchoni wrote:
> On 11/24/2017 02:09 AM, Andrew Bartlett via samba-technical wrote:
> > I don't really like
> >
> > [PATCH v4 01/13] s4-cli: increment once in the for loop
>
> What do you have in mind? I agree with the compile that incrementing
> both in the for-loop and inside the loop is not very good - people
> looking at the code might miss the increment. Shall we transform it into
> a while loop?

I'm not sure.  In trying to get as much of your patches in so a
reasonable discussion could be had of the remainder, I've set a high
bar for 'I clearly understand what this patch is trying to do and why'
and a high sensitivity for 'gut feeling'.  

It isn't an objection in that sense, mostly me putting it off hoping
someone else solves it for me.  

> > I would like some broader comment on:
> >
> > [PATCH v4 08/13] build: remove -Wcast-align from developer build
> >
>
> That commit message was already considerably longer than the average
> commit message. Following is a broader commit message, please tell if
> this is closer to what you had in mind.

It is more that I would like comment from someone like metze before
removing a check we thought we had 'just' to build on a new compiler.  

I like the explanation, and agree with you, but I as there isn't a rush
I'm looking for broader comment.

Also, if this rule is violated on these platforms, does the kernel fix
it up or is this SIGBUS?

Thanks,

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: -Wcast-qual Re: [PATCHES v4] Another round of FreeBSD developer build fixes

Samba - samba-technical mailing list
>>> I would like some broader comment on:
>>>
>>> [PATCH v4 08/13] build: remove -Wcast-align from developer build

Please don't remove -Wcast-align! The following should allow the build:

ADDITIONAL_CFLAGS="-Wno-error=cast-align" ./configure.developer
--picky-developer

We use something similar with -Wno-error=deprecated-declarations.

metze


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: -Wcast-qual Re: [PATCHES v4] Another round of FreeBSD developer build fixes

Samba - samba-technical mailing list
On Sat, Nov 25, 2017 at 01:02:28PM +0100, Stefan Metzmacher via samba-technical wrote:

> >>> I would like some broader comment on:
> >>>
> >>> [PATCH v4 08/13] build: remove -Wcast-align from developer build
>
> Please don't remove -Wcast-align! The following should allow the build:
>
> ADDITIONAL_CFLAGS="-Wno-error=cast-align" ./configure.developer
> --picky-developer
>
> We use something similar with -Wno-error=deprecated-declarations.

Genuine question: How do we deal with that properly? A
standards-compliant way might be a memcpy to a variable on the stack,
but this might be very inefficient for large structures. Also, for
example in talloc.c we inevitably do some casting to structs. How is
that possible in a portable manner that does not generate warnings?

Volker

>
> metze
>




--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: -Wcast-qual Re: [PATCHES v4] Another round of FreeBSD developer build fixes

Samba - samba-technical mailing list
On Sat, Nov 25, 2017 at 02:02:43PM +0100, Volker Lendecke via samba-technical wrote:
> Genuine question: How do we deal with that properly? A
> standards-compliant way might be a memcpy to a variable on the stack,
> but this might be very inefficient for large structures. Also, for
> example in talloc.c we inevitably do some casting to structs. How is
> that possible in a portable manner that does not generate warnings?

Just because it's fun to try: http://orchistro.tistory.com/206 has

__asm__("pushf\norl $0x40000, (%rsp)\npopf");

which makes talloc_strdup on my 64-bit debian stretch immediately
sigbus in memcpy:

#0  __memmove_avx_unaligned_erms ()
    at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:306
#1  0x00007ffff5ceed16 in __talloc_strlendup (t=0x55555583a260,
    p=0x7ffff4adaaa0 "all", len=3) at ../lib/talloc/talloc.c:2372
#2  0x00007ffff5ceec89 in talloc_strdup (t=0x55555583a260,
    p=0x7ffff4adaaa0 "all") at ../lib/talloc/talloc.c:2385
#3  0x00007ffff4ad82f1 in debug_add_class (classname=0x7ffff4adaaa0 "all")
    at ../lib/util/debug.c:706
#4  0x00007ffff4ad838d in debug_init () at ../lib/util/debug.c:864
#5  0x00007ffff4ad9384 in setup_logging (
    prog_name=0x5555555dcb4f "smbtorture", new_logtype=DEBUG_STDOUT)
    at ../lib/util/debug.c:922
#6  0x0000555555574124 in main (argc=3, argv=0x7fffffffe478)
    at ../source3/torture/torture.c:11703

So there would be quite some work to fix the real problems that this
warning might point out :-)

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: -Wcast-qual Re: [PATCHES v4] Another round of FreeBSD developer build fixes

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On 11/25/2017 02:02 PM, Stefan Metzmacher wrote:
>>>> I would like some broader comment on:
>>>>
>>>> [PATCH v4 08/13] build: remove -Wcast-align from developer build
>
> Please don't remove -Wcast-align! The following should allow the build:
>
> ADDITIONAL_CFLAGS="-Wno-error=cast-align" ./configure.developer
> --picky-developer
>
It's possible of course. My aim has been to succeed without having to
add flags for a native build on a rather mainstream OS.

I would appreciate some reasoning added to not removing this flag, esp
after I've explained in a rather longish way why IMO this flag either
does nothing, or breaks the build (depending on platform).

> We use something similar with -Wno-error=deprecated-declarations.
>
> metze
>

Andrew, Re: what misalignment causes - on Linux it's configurable
through /proc/cpu/alignment, and there's an option for the kernel to fix
it up. In ARM/MIPS projects I was involved with, we would set it to send
a SIGBUG, not wanting to live with broken code.

Thanks,
Uri.

Reply | Threaded
Open this post in threaded view
|

Re: -Wcast-qual Re: [PATCHES v4] Another round of FreeBSD developer build fixes

Samba - samba-technical mailing list
Uri, thanks again for your patches and work. I managed finally to compile
HEAD with --picky-developer with only few uncommitted patches.

So, we still need: [PATCH 01/24] sysacls: make SMB_ACL_PERMSET_T opaque

Without it compilation fails with:

../source3/lib/sysacls.c:98:13: error: incompatible pointer types assigning
to 'SMB_ACL_PERMSET_T' (aka 'unsigned short *') from 'uint32_t *' (aka
'unsigned int *')
      [-Werror,-Wincompatible-pointer-types]
        *permset_p = &entry_d->a_perm;
                   ^ ~~~~~~~~~~~~~~~~
1 error generated.

But that's was already known.

pam_wrapper seems didn't need the "[PATCH v4 10/13] pam_wrapper: use
uintptr_t as base for const-discarding", at least for FreeBSD, as both are
detected:
#define HAVE_INTPTR_T 1
#define HAVE_UINTPTR_T 1

What is missing is smth. like:

--- third_party/pam_wrapper/libpamtest.h.orig      2017-11-26
03:55:52.608871000 +0100
+++ third_party/pam_wrapper/libpamtest.h   2017-11-26 03:56:07.925369000
+0100
@@ -21,6 +21,7 @@

 #include <stdint.h>
 #include <security/pam_appl.h>
+#include "config.h"

 /**
  * @defgroup pamtest The pamtest API


And last, but not least set of errors that pop up are related to
pam_winbind:

../nsswitch/pam_winbind.c:201:16: error: format string is not a string
literal [-Werror,-Wformat-nonliteral]
                vsyslog(err, format, args);
                             ^~~~~~
../nsswitch/pam_winbind.c:205:15: error: format string is not a string
literal [-Werror,-Wformat-nonliteral]
        vsyslog(err, format2, args);
                     ^~~~~~~
../nsswitch/pam_winbind.c:722:24: error: format string is not a string
literal [-Werror,-Wformat-nonliteral]
        ret = vasprintf(&var, format, args);
                              ^~~~~~
3 errors generated.

Two of them are easily fixable with:
--- nsswitch/pam_winbind.c.orig    2017-11-25 14:13:23.000000000 +0100
+++ nsswitch/pam_winbind.c 2017-11-26 03:41:50.271660000 +0100
@@ -175,6 +175,11 @@ static inline void textdomain_init(void)

 /* some syslogging */

+static void _pam_log_int(const pam_handle_t *pamh,
+                        int err,
+                        const char *format,
+                        va_list args) PRINTF_ATTRIBUTE(3,0);
+
 #ifdef HAVE_PAM_VSYSLOG
 static void _pam_log_int(const pam_handle_t *pamh,
                         int err,
@@ -714,6 +719,11 @@ static int _make_remark(struct pwb_conte
 static int _make_remark_v(struct pwb_context *ctx,
                          int type,
                          const char *format,
+                         va_list args) PRINTF_ATTRIBUTE(3,0);
+
+static int _make_remark_v(struct pwb_context *ctx,
+                         int type,
+                         const char *format,
                          va_list args)
 {
        char *var;


But not the one on line 205.

With regards,
Timur Bakeyev.


On Sat, Nov 25, 2017 at 2:52 PM, Uri Simchoni <[hidden email]> wrote:

> On 11/25/2017 02:02 PM, Stefan Metzmacher wrote:
> >>>> I would like some broader comment on:
> >>>>
> >>>> [PATCH v4 08/13] build: remove -Wcast-align from developer build
> >
> > Please don't remove -Wcast-align! The following should allow the build:
> >
> > ADDITIONAL_CFLAGS="-Wno-error=cast-align" ./configure.developer
> > --picky-developer
> >
> It's possible of course. My aim has been to succeed without having to
> add flags for a native build on a rather mainstream OS.
>
> I would appreciate some reasoning added to not removing this flag, esp
> after I've explained in a rather longish way why IMO this flag either
> does nothing, or breaks the build (depending on platform).
>
> > We use something similar with -Wno-error=deprecated-declarations.
> >
> > metze
> >
>
> Andrew, Re: what misalignment causes - on Linux it's configurable
> through /proc/cpu/alignment, and there's an option for the kernel to fix
> it up. In ARM/MIPS projects I was involved with, we would set it to send
> a SIGBUG, not wanting to live with broken code.
>
> Thanks,
> Uri.
>
Reply | Threaded
Open this post in threaded view
|

Re: -Wcast-qual Re: [PATCHES v4] Another round of FreeBSD developer build fixes

Samba - samba-technical mailing list
On 11/26/2017 05:21 AM, Timur I. Bakeyev via samba-technical wrote:
> And last, but not least set of errors that pop up are related to
> pam_winbind:
>

I already have a fix for pam_winbind that meets the "no pragma" policy -
see attached. I just haven't gotten to testing it on Linux yet.

I'm still contemplating on the sysacls stuff which is the remaining issue.

Thanks,
Uri.

pam_winbind.patch.txt (2K) Download Attachment