vfs_zfsacl.c: use of undeclared identifier 'ace'

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

vfs_zfsacl.c: use of undeclared identifier 'ace'

Samba - samba-technical mailing list
Reply | Threaded
Open this post in threaded view
|

Re: vfs_zfsacl.c: use of undeclared identifier 'ace'

Samba - samba-technical mailing list
On Sat, Nov 11, 2017 at 02:40:50PM +0000, Youzhong Yang via samba-technical wrote:
> Where is 'ace' defined?
>
> https://github.com/samba-team/samba/blob/v4-7-stable/source3/modules/vfs_zfsacl.c#L69

<https://git.samba.org/?p=samba.git;a=commitdiff;h=11da1e5c056c92fd7f51ecce0285628cac65f174>

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

Reply | Threaded
Open this post in threaded view
|

RE: vfs_zfsacl.c: use of undeclared identifier 'ace'

Samba - samba-technical mailing list
Thanks, it fixes the compilation error.

I appreciate Samba team's efforts/contributions. But as an outsider, just a little bit concerned - it seems code change/fix is so easy to get into the system, without adequate review/testing.

Thanks,

--Youzhong

-----Original Message-----
From: Ralph Böhme [mailto:[hidden email]]
Sent: Saturday, November 11, 2017 9:47 AM
To: Youzhong Yang <[hidden email]>
Cc: [hidden email]
Subject: Re: vfs_zfsacl.c: use of undeclared identifier 'ace'

On Sat, Nov 11, 2017 at 02:40:50PM +0000, Youzhong Yang via samba-technical wrote:
> Where is 'ace' defined?
>
> https://github.com/samba-team/samba/blob/v4-7-stable/source3/modules/vfs_zfsacl.c#L69

<https://git.samba.org/?p=samba.git;a=commitdiff;h=11da1e5c056c92fd7f51ecce0285628cac65f174>

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/
Reply | Threaded
Open this post in threaded view
|

Re: vfs_zfsacl.c: use of undeclared identifier 'ace'

Samba - samba-technical mailing list
On Sat, Nov 11, 2017 at 11:39:40PM +0000, Youzhong Yang via samba-technical wrote:
> Thanks, it fixes the compilation error.
>
> I appreciate Samba team's efforts/contributions. But as an outsider, just a little bit concerned - it seems code change/fix is so easy to get into the system, without adequate review/testing.

Not really. zfsacls are not shipped on the majority of
platforms, so it's hard to have regression tests for code
that most people can't compile.

It's impossible to get compilation errors into mainline where regression
tests ensure quality.

Reply | Threaded
Open this post in threaded view
|

Re: vfs_zfsacl.c: use of undeclared identifier 'ace'

Samba - samba-technical mailing list
You are both right.

If it isn't on Linux, it isn't well tested, IMHO.

I faced this all the time when I worked on similar projects to Youzhong.

So yes.... things can be checked in that break the build on *insert
non-linux os* here near trivially.

I remember having to clean it up every few months, back when I did that.

Thankfully, the core code tends to work well, even outside of Linux, or at
least it used to.

Thanks,

-Ira

On Mon, Nov 13, 2017 at 1:25 PM, Jeremy Allison via samba-technical <
[hidden email]> wrote:

> On Sat, Nov 11, 2017 at 11:39:40PM +0000, Youzhong Yang via
> samba-technical wrote:
> > Thanks, it fixes the compilation error.
> >
> > I appreciate Samba team's efforts/contributions. But as an outsider,
> just a little bit concerned - it seems code change/fix is so easy to get
> into the system, without adequate review/testing.
>
> Not really. zfsacls are not shipped on the majority of
> platforms, so it's hard to have regression tests for code
> that most people can't compile.
>
> It's impossible to get compilation errors into mainline where regression
> tests ensure quality.
>
>
Reply | Threaded
Open this post in threaded view
|

RE: vfs_zfsacl.c: use of undeclared identifier 'ace'

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
I don't disagree. But FreeBSD does ship zfsacl.so:

/usr/local/lib/shared-modules/vfs/zfsacl.so

Last month or so I've been playing with Linux (Centos, Ubuntu) and FreeBSD, in terms of tracing and debugging, Solaris/illumos is still unbeatable. If FreeBSD can have 'mdb' integrated, it would be very attractive. I know someone is working on it ...

Anyway, it's not a place to debate which OS is good or bad. For Solaris/illumos, can I volunteer to be the tester? We can try to have some retired hardware dedicated for this purpose. I'd like to try my best to make this happen, although I cannot guarantee the resource and my time. Please let me know.

Best,

--Youzhong

-----Original Message-----
From: Jeremy Allison [mailto:[hidden email]]
Sent: Monday, November 13, 2017 1:25 PM
To: Youzhong Yang <[hidden email]>
Cc: Ralph Böhme <[hidden email]>; [hidden email]
Subject: Re: vfs_zfsacl.c: use of undeclared identifier 'ace'

On Sat, Nov 11, 2017 at 11:39:40PM +0000, Youzhong Yang via samba-technical wrote:
> Thanks, it fixes the compilation error.
>
> I appreciate Samba team's efforts/contributions. But as an outsider, just a little bit concerned - it seems code change/fix is so easy to get into the system, without adequate review/testing.

Not really. zfsacls are not shipped on the majority of platforms, so it's hard to have regression tests for code that most people can't compile.

It's impossible to get compilation errors into mainline where regression tests ensure quality.

Reply | Threaded
Open this post in threaded view
|

Re: vfs_zfsacl.c: use of undeclared identifier 'ace'

Samba - samba-technical mailing list
On Mon, Nov 13, 2017 at 07:02:32PM +0000, Youzhong Yang wrote:
> I don't disagree. But FreeBSD does ship zfsacl.so:
>
> /usr/local/lib/shared-modules/vfs/zfsacl.so
>
> Last month or so I've been playing with Linux (Centos, Ubuntu) and FreeBSD, in terms of tracing and debugging, Solaris/illumos is still unbeatable. If FreeBSD can have 'mdb' integrated, it would be very attractive. I know someone is working on it ...
>
> Anyway, it's not a place to debate which OS is good or bad. For Solaris/illumos, can I volunteer to be the tester? We can try to have some retired hardware dedicated for this purpose. I'd like to try my best to make this happen, although I cannot guarantee the resource and my time. Please let me know.

No, I'm not passing judgement, other than saying that
the fact we can't get RichACLs into the Linux kernel
due to one intransigent developer is a f%&king disgrace :-(.

I'm happy for you to keep riding us hard on FreeBSD/Illumos
breakage (although less on the Illumos - now Oracle killed
Solaris the pale, hungry children are doomed to die a slow,
ignominious death :-).

But without auto-regression tests on FreeBSD triggered on
every code submission, we are going to get occasional
compile breakage, and I don't see a way around that.

Reply | Threaded
Open this post in threaded view
|

Re: vfs_zfsacl.c: use of undeclared identifier 'ace'

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

On Mon, Nov 13, 2017 at 8:08 PM, Jeremy Allison via samba-technical <
[hidden email]> wrote:

>
> But without auto-regression tests on FreeBSD triggered on
> every code submission, we are going to get occasional
> compile breakage, and I don't see a way around that


Can I offer iXsystems Inc. CI and build systems for Samba project to have
CI and coverage tests so bugs like this won't slip into the release code?

With best regards,
Timur Bakeyev,
iXsystems Inc.
Reply | Threaded
Open this post in threaded view
|

Re: vfs_zfsacl.c: use of undeclared identifier 'ace'

Samba - samba-technical mailing list
On Sat, Nov 18, 2017 at 12:15:05AM +0100, Timur I. Bakeyev wrote:

> Hi, Jeremy!
>
> On Mon, Nov 13, 2017 at 8:08 PM, Jeremy Allison via samba-technical <
> [hidden email]> wrote:
>
>
>     But without auto-regression tests on FreeBSD triggered on
>     every code submission, we are going to get occasional
>     compile breakage, and I don't see a way around that
>
>
> Can I offer iXsystems Inc. CI and build systems for Samba project to have CI
> and coverage tests so bugs like this won't slip into the release code?

This requires a lot of coordination to work. Right now, Team members
sit at their local boxes and type:

git autobuild

in the tree they want to push to master. This psuhes to a defined
tree on sn-devel, and runs a set of scripts that make everything,
run the regression tests suites, and if everything passes do the
final migration of the code into the master git repo.

If anything fails there are web-pages you can look at to see
what test or part of the compile failed.

You'd need to duplicate that environment, and we'd need some
sort of a hook to ensure build and tests succeed on both
Linux and FreeBSD before a real commit.

Right now, can you type:

make test

on a FreeBSD Samba build and have all the tests pass ? I
would currently doubt it, as many of the tests (like the
Linux kernel oplock and change notify ones) require Linux
specific features. Also, some of the tevent tests require
epoll etc. etc.

This is doable, but not an easy change to make.

Reply | Threaded
Open this post in threaded view
|

CI beyond sn-devel

Samba - samba-technical mailing list
On Fri, 2017-11-17 at 15:35 -0800, Jeremy Allison via samba-technical
wrote:

> On Sat, Nov 18, 2017 at 12:15:05AM +0100, Timur I. Bakeyev wrote:
> > Hi, Jeremy!
> >
> > On Mon, Nov 13, 2017 at 8:08 PM, Jeremy Allison via samba-technical <
> > [hidden email]> wrote:
> >
> >
> >     But without auto-regression tests on FreeBSD triggered on
> >     every code submission, we are going to get occasional
> >     compile breakage, and I don't see a way around that
> >
> >
> > Can I offer iXsystems Inc. CI and build systems for Samba project to have CI
> > and coverage tests so bugs like this won't slip into the release code?
>
> This requires a lot of coordination to work. Right now, Team members
> sit at their local boxes and type:
>
> git autobuild
>
> in the tree they want to push to master. This psuhes to a defined
> tree on sn-devel, and runs a set of scripts that make everything,
> run the regression tests suites, and if everything passes do the
> final migration of the code into the master git repo.
>
> If anything fails there are web-pages you can look at to see
> what test or part of the compile failed.
>
> You'd need to duplicate that environment, and we'd need some
> sort of a hook to ensure build and tests succeed on both
> Linux and FreeBSD before a real commit.

I agree with the sentiment and goals, but I actually think we need to
move beyond (wrap) autobuild.  It can't scale beyond the host it runs
on, and we need to move to something less bespoke.  

Have a look at this: Homu integrates with github (which we don't use as
a primary platform as it is not free software), but does a lot of what
we need, specifically one-at-a-time rebase and test:

https://github.com/servo/homu

If we had something like that, hooked up to a gitlab and a cloud-hosted
CI system such as gitabl-ci then we can test beyond just one OS
(indeed, one host), and reject commits that break FreeBSD, are
incompatible with Windows or fail the WSPP testsuite.

> Right now, can you type:
>
> make test
>
> on a FreeBSD Samba build and have all the tests pass ? I
> would currently doubt it, as many of the tests (like the
> Linux kernel oplock and change notify ones) require Linux
> specific features. Also, some of the tevent tests require
> epoll etc. etc.
>
> This is doable, but not an easy change to make.

I think this is the best place to start.  Document the current failures
in a new knownfail.d file as a start.

In terms of resources, one think that iXsystems may be able to help
with is target hosts for the cloud CI I mention above, or paying for
some cloud time.  

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: CI beyond sn-devel

Samba - samba-technical mailing list
On la, 18 marras 2017, Andrew Bartlett via samba-technical wrote:

> On Fri, 2017-11-17 at 15:35 -0800, Jeremy Allison via samba-technical
> wrote:
> > On Sat, Nov 18, 2017 at 12:15:05AM +0100, Timur I. Bakeyev wrote:
> > > Hi, Jeremy!
> > >
> > > On Mon, Nov 13, 2017 at 8:08 PM, Jeremy Allison via samba-technical <
> > > [hidden email]> wrote:
> > >
> > >
> > >     But without auto-regression tests on FreeBSD triggered on
> > >     every code submission, we are going to get occasional
> > >     compile breakage, and I don't see a way around that
> > >
> > >
> > > Can I offer iXsystems Inc. CI and build systems for Samba project to have CI
> > > and coverage tests so bugs like this won't slip into the release code?
> >
> > This requires a lot of coordination to work. Right now, Team members
> > sit at their local boxes and type:
> >
> > git autobuild
> >
> > in the tree they want to push to master. This psuhes to a defined
> > tree on sn-devel, and runs a set of scripts that make everything,
> > run the regression tests suites, and if everything passes do the
> > final migration of the code into the master git repo.
> >
> > If anything fails there are web-pages you can look at to see
> > what test or part of the compile failed.
> >
> > You'd need to duplicate that environment, and we'd need some
> > sort of a hook to ensure build and tests succeed on both
> > Linux and FreeBSD before a real commit.
>
> I agree with the sentiment and goals, but I actually think we need to
> move beyond (wrap) autobuild.  It can't scale beyond the host it runs
> on, and we need to move to something less bespoke.  
>
> Have a look at this: Homu integrates with github (which we don't use as
> a primary platform as it is not free software), but does a lot of what
> we need, specifically one-at-a-time rebase and test:
>
> https://github.com/servo/homu
>
> If we had something like that, hooked up to a gitlab and a cloud-hosted
> CI system such as gitabl-ci then we can test beyond just one OS
> (indeed, one host), and reject commits that break FreeBSD, are
> incompatible with Windows or fail the WSPP testsuite.

We have a similar model with freeIPA: https://github.com/freeipa/freeipa-pr-ci/
 - a master git repository is hosted at pagure.io

 - a commit hook updates github repository with force-push, making that
   one a mirror

 - any pull request proposed against the github tree either by an
   authorized person or having a label to run a test by an authorized
   person triggers a pull request CI to run tests

 - these tests run on handlers that may be set up everywhere else; in
   freeIPA case we have official PR CI running on a Red Hat internal
   servers and on travis-ci infrastructure

 - once PR CI runs completed, their status is marked in the github pull
   request

 - once pull request is ACKed by a human, the code from PR can be merged
   to the master tree on pagure.io by a human. We use some scripts to
   automate this process -- the script checks PR status, its PR CI
   status, whether it can be applied by fast forward to a selected git
   branch and also is able to open pull requests for backports
   automatically

github is basically used in this scheme just to ease the process to
provide pull requests for contributors and to visualize results of PR CI
integration. The whole story starts with a git push to a remote place
and marking this push as 'authorized for PR CI run', this is equivalent
to 'git autobuild' in the current Samba Team practice. One can easily
keep these models compatible because all it really requires to have is a
place where git remotes are pushed and a way to mark those pushes as
'authorized'. A PR CI service could be pushed with builds coming from
anywhere and 'git autobuild' can still be used as an entry point to push
the changes.

Finally, one major difference to current Samba Team practice is that
autobuilds with PR CI would be public before they complete as the trees
need to be copied to a test runner somehow. Right now the tree being
tested is kept private until it the test succeeded and merged. We also
have private autobuilds that never get merged and just run to see a
success or a failure. This is important for security releases
coordination. So any such PR CI system would need to account for a need
of being run from private pushes and not publishing logs of those until
allowed (or allow seeing them under some account privileges).

I have freeIPA PR CI running also against my personal freeIPA clone on
github. That PR CI is using my own freeIPA PR CI runner instance on my
desktop. So every time I push something and propose as a clone to
myself, I trigger the test runs. Here is an example:
https://github.com/abbra/freeipa/pull/5.  Scroll down to "All checks
passed" to see four different scenarious that were executed and links to
actual logs. The logs are actually pushed to some Fedora hosted server
which is used as a logs storage for each build.

> > would currently doubt it, as many of the tests (like the
> > Linux kernel oplock and change notify ones) require Linux
> > specific features. Also, some of the tevent tests require
> > epoll etc. etc.
> >
> > This is doable, but not an easy change to make.
>
> I think this is the best place to start.  Document the current failures
> in a new knownfail.d file as a start.
>
> In terms of resources, one think that iXsystems may be able to help
> with is target hosts for the cloud CI I mention above, or paying for
> some cloud time.  
--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|

Re: vfs_zfsacl.c: use of undeclared identifier 'ace'

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On 11/18/2017 01:35 AM, Jeremy Allison via samba-technical wrote:

>
> Right now, can you type:
>
> make test
>
> on a FreeBSD Samba build and have all the tests pass ? I
> would currently doubt it, as many of the tests (like the
> Linux kernel oplock and change notify ones) require Linux
> specific features. Also, some of the tevent tests require
> epoll etc. etc.
>
As step 0.1, here's a patch set that make Samba build successfully in
developer mode on FreeBSD 11.1-RELEASE (clang 4.0.0). Most of this is
clang-related, so I suppose it has value beyond FreeBSD.

The configuration command I used was:

ADDITIONAL_CFLAGS="-Wno-unknown-warning-option" ./configure.developer
--with-selftest-prefix=./bin/ab  --picky-developer --abi-check-disable
--prefix=<my prefix> --with-profiling-data

This is *almost* the Samba autobuild configuration, except:
1. No ABI checking - It looks like ABI checking depends on some tool
that emits differently-formatted output on FreeBSD

2. The no-unknonwn-warning-option - I couldn't figure out how to make
clang, during configuration time, identify whether it supports a warning
option or not, *without* supplying other command line options which
could bread another compiler. Upstream waf simply identifies clang, so
one might use that to insert clang-specific behavior.

I'm not sure when (and if...) I'll be able to continue this and see what
are the make test hurdles. If we can hook this to some CI, we can detect
post-push build breakage and fix it before release.

Thanks,
Uri.

> This is doable, but not an easy change to make.
>


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

Re: vfs_zfsacl.c: use of undeclared identifier 'ace'

Samba - samba-technical mailing list
Thanks, Uri, a great work have been done to chaise all this annoying
warnings. Hope those will end up in the master tree.

In you "[PATCH 16/35] winbind_nss_freebsd: fix a const discard warning"
though you change the signature of the function:

int
__getgroupmembership(const char *uname, gid_t agroup, gid_t *groups,
        int maxgrp, int *grpcnt)

which should be avoided. Need to fix this warning somewhere else.

As for the Clang being liberal regarding unknown options we use the
following trick:

--- buildtools/wafsamba/samba_autoconf.py.orig  2017-07-04 10:05:25 UTC
+++ buildtools/wafsamba/samba_autoconf.py
@@ -873,7 +873,7 @@ def SAMBA_CHECK_UNDEFINED_SYMBOL_FLAGS(c
         conf.env.undefined_ldflags = conf.ADD_LDFLAGS('-Wl,-no-undefined',
testflags=True)

     if not sys.platform.startswith("openbsd") and
conf.env.undefined_ignore_ldflags == []:
-        if conf.CHECK_LDFLAGS(['-undefined', 'dynamic_lookup']):
+        if conf.CHECK_LDFLAGS(['-undefined', 'dynamic_lookup'] +
conf.env.WERROR_CFLAGS):
             conf.env.undefined_ignore_ldflags = ['-undefined',
'dynamic_lookup']

 @conf
--- lib/replace/wscript.orig    2017-11-02 11:38:36 UTC
+++ lib/replace/wscript
@@ -81,7 +81,7 @@ def configure(conf):
     conf.CHECK_HEADERS('sys/atomic.h')
     conf.CHECK_HEADERS('libgen.h')

-    if conf.CHECK_CFLAGS('-Wno-format-truncation'):
+    if conf.CHECK_CFLAGS(['-Wno-format-truncation'] +
conf.env.WERROR_CFLAGS):
         conf.define('HAVE_WNO_FORMAT_TRUNCATION', '1')

     # Check for process set name support


There are a lot of small fixes like this in the FreeBSD Samba port, hope
they'll find their way into master as well.

With regards,
Timur Bakeyev.

On Mon, Nov 20, 2017 at 10:10 AM, Uri Simchoni <[hidden email]> wrote:

> On 11/18/2017 01:35 AM, Jeremy Allison via samba-technical wrote:
> >
> > Right now, can you type:
> >
> > make test
> >
> > on a FreeBSD Samba build and have all the tests pass ? I
> > would currently doubt it, as many of the tests (like the
> > Linux kernel oplock and change notify ones) require Linux
> > specific features. Also, some of the tevent tests require
> > epoll etc. etc.
> >
>
> As step 0.1, here's a patch set that make Samba build successfully in
> developer mode on FreeBSD 11.1-RELEASE (clang 4.0.0). Most of this is
> clang-related, so I suppose it has value beyond FreeBSD.
>
> The configuration command I used was:
>
> ADDITIONAL_CFLAGS="-Wno-unknown-warning-option" ./configure.developer
> --with-selftest-prefix=./bin/ab  --picky-developer --abi-check-disable
> --prefix=<my prefix> --with-profiling-data
>
> This is *almost* the Samba autobuild configuration, except:
> 1. No ABI checking - It looks like ABI checking depends on some tool
> that emits differently-formatted output on FreeBSD
>
> 2. The no-unknonwn-warning-option - I couldn't figure out how to make
> clang, during configuration time, identify whether it supports a warning
> option or not, *without* supplying other command line options which
> could bread another compiler. Upstream waf simply identifies clang, so
> one might use that to insert clang-specific behavior.
>
> I'm not sure when (and if...) I'll be able to continue this and see what
> are the make test hurdles. If we can hook this to some CI, we can detect
> post-push build breakage and fix it before release.
>
> Thanks,
> Uri.
>
> > This is doable, but not an easy change to make.
> >
>
>
Reply | Threaded
Open this post in threaded view
|

[PATCHES v2] Fix FreeBSD developer build (was: Re: vfs_zfsacl.c: use of undeclared identifier 'ace')

Samba - samba-technical mailing list
Attached v2, with 16/35 modified so as not to change the params.

As for the cflag detection tricks - What I tried changed things on the
Linux/gcc side, so I propose to leave it at that (with the
ADDITIONAL_CFLAGS workaround) and attack this later.

Thanks,
Uri.

On 11/20/2017 03:26 PM, Timur I. Bakeyev via samba-technical wrote:

> Thanks, Uri, a great work have been done to chaise all this annoying
> warnings. Hope those will end up in the master tree.
>
> In you "[PATCH 16/35] winbind_nss_freebsd: fix a const discard warning"
> though you change the signature of the function:
>
> int
> __getgroupmembership(const char *uname, gid_t agroup, gid_t *groups,
>         int maxgrp, int *grpcnt)
>
> which should be avoided. Need to fix this warning somewhere else.
>
> As for the Clang being liberal regarding unknown options we use the
> following trick:
>
> --- buildtools/wafsamba/samba_autoconf.py.orig  2017-07-04 10:05:25 UTC
> +++ buildtools/wafsamba/samba_autoconf.py
> @@ -873,7 +873,7 @@ def SAMBA_CHECK_UNDEFINED_SYMBOL_FLAGS(c
>          conf.env.undefined_ldflags = conf.ADD_LDFLAGS('-Wl,-no-undefined',
> testflags=True)
>
>      if not sys.platform.startswith("openbsd") and
> conf.env.undefined_ignore_ldflags == []:
> -        if conf.CHECK_LDFLAGS(['-undefined', 'dynamic_lookup']):
> +        if conf.CHECK_LDFLAGS(['-undefined', 'dynamic_lookup'] +
> conf.env.WERROR_CFLAGS):
>              conf.env.undefined_ignore_ldflags = ['-undefined',
> 'dynamic_lookup']
>
>  @conf
> --- lib/replace/wscript.orig    2017-11-02 11:38:36 UTC
> +++ lib/replace/wscript
> @@ -81,7 +81,7 @@ def configure(conf):
>      conf.CHECK_HEADERS('sys/atomic.h')
>      conf.CHECK_HEADERS('libgen.h')
>
> -    if conf.CHECK_CFLAGS('-Wno-format-truncation'):
> +    if conf.CHECK_CFLAGS(['-Wno-format-truncation'] +
> conf.env.WERROR_CFLAGS):
>          conf.define('HAVE_WNO_FORMAT_TRUNCATION', '1')
>
>      # Check for process set name support
>
>
> There are a lot of small fixes like this in the FreeBSD Samba port, hope
> they'll find their way into master as well.
>
> With regards,
> Timur Bakeyev.
>
> On Mon, Nov 20, 2017 at 10:10 AM, Uri Simchoni <[hidden email]> wrote:
>
>> On 11/18/2017 01:35 AM, Jeremy Allison via samba-technical wrote:
>>>
>>> Right now, can you type:
>>>
>>> make test
>>>
>>> on a FreeBSD Samba build and have all the tests pass ? I
>>> would currently doubt it, as many of the tests (like the
>>> Linux kernel oplock and change notify ones) require Linux
>>> specific features. Also, some of the tevent tests require
>>> epoll etc. etc.
>>>
>>
>> As step 0.1, here's a patch set that make Samba build successfully in
>> developer mode on FreeBSD 11.1-RELEASE (clang 4.0.0). Most of this is
>> clang-related, so I suppose it has value beyond FreeBSD.
>>
>> The configuration command I used was:
>>
>> ADDITIONAL_CFLAGS="-Wno-unknown-warning-option" ./configure.developer
>> --with-selftest-prefix=./bin/ab  --picky-developer --abi-check-disable
>> --prefix=<my prefix> --with-profiling-data
>>
>> This is *almost* the Samba autobuild configuration, except:
>> 1. No ABI checking - It looks like ABI checking depends on some tool
>> that emits differently-formatted output on FreeBSD
>>
>> 2. The no-unknonwn-warning-option - I couldn't figure out how to make
>> clang, during configuration time, identify whether it supports a warning
>> option or not, *without* supplying other command line options which
>> could bread another compiler. Upstream waf simply identifies clang, so
>> one might use that to insert clang-specific behavior.
>>
>> I'm not sure when (and if...) I'll be able to continue this and see what
>> are the make test hurdles. If we can hook this to some CI, we can detect
>> post-push build breakage and fix it before release.
>>
>> Thanks,
>> Uri.
>>
>>> This is doable, but not an easy change to make.
>>>
>>
>>


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

Re: [PATCHES v2] Fix FreeBSD developer build (was: Re: vfs_zfsacl.c: use of undeclared identifier 'ace')

Samba - samba-technical mailing list
Hi, Uri!

That's quite a jumbo patch, possibly splitting it into smaller chunks could
be easier to review :)

I've noticed mentioning of missing HAVE_INTPTR_T on FreeBSD, need to check
that, but meanwhile:

https://bugzilla.samba.org/show_bug.cgi?id=4635

Also, is that patchset against the HEAD? As traditionally we usually first
work with releases, so at least two patches fail to me...

What are the problems with the patches I sent on Linux? I believe similar
approach is used in couple of other places in wscripts.

With regards,
Timur Bakeyev.





On Mon, Nov 20, 2017 at 11:00 PM, Uri Simchoni <[hidden email]> wrote:

> Attached v2, with 16/35 modified so as not to change the params.
>
> As for the cflag detection tricks - What I tried changed things on the
> Linux/gcc side, so I propose to leave it at that (with the
> ADDITIONAL_CFLAGS workaround) and attack this later.
>
> Thanks,
> Uri.
>
> On 11/20/2017 03:26 PM, Timur I. Bakeyev via samba-technical wrote:
> > Thanks, Uri, a great work have been done to chaise all this annoying
> > warnings. Hope those will end up in the master tree.
> >
> > In you "[PATCH 16/35] winbind_nss_freebsd: fix a const discard warning"
> > though you change the signature of the function:
> >
> > int
> > __getgroupmembership(const char *uname, gid_t agroup, gid_t *groups,
> >         int maxgrp, int *grpcnt)
> >
> > which should be avoided. Need to fix this warning somewhere else.
> >
> > As for the Clang being liberal regarding unknown options we use the
> > following trick:
> >
> > --- buildtools/wafsamba/samba_autoconf.py.orig  2017-07-04 10:05:25 UTC
> > +++ buildtools/wafsamba/samba_autoconf.py
> > @@ -873,7 +873,7 @@ def SAMBA_CHECK_UNDEFINED_SYMBOL_FLAGS(c
> >          conf.env.undefined_ldflags = conf.ADD_LDFLAGS('-Wl,-no-
> undefined',
> > testflags=True)
> >
> >      if not sys.platform.startswith("openbsd") and
> > conf.env.undefined_ignore_ldflags == []:
> > -        if conf.CHECK_LDFLAGS(['-undefined', 'dynamic_lookup']):
> > +        if conf.CHECK_LDFLAGS(['-undefined', 'dynamic_lookup'] +
> > conf.env.WERROR_CFLAGS):
> >              conf.env.undefined_ignore_ldflags = ['-undefined',
> > 'dynamic_lookup']
> >
> >  @conf
> > --- lib/replace/wscript.orig    2017-11-02 11:38:36 UTC
> > +++ lib/replace/wscript
> > @@ -81,7 +81,7 @@ def configure(conf):
> >      conf.CHECK_HEADERS('sys/atomic.h')
> >      conf.CHECK_HEADERS('libgen.h')
> >
> > -    if conf.CHECK_CFLAGS('-Wno-format-truncation'):
> > +    if conf.CHECK_CFLAGS(['-Wno-format-truncation'] +
> > conf.env.WERROR_CFLAGS):
> >          conf.define('HAVE_WNO_FORMAT_TRUNCATION', '1')
> >
> >      # Check for process set name support
> >
> >
> > There are a lot of small fixes like this in the FreeBSD Samba port, hope
> > they'll find their way into master as well.
> >
> > With regards,
> > Timur Bakeyev.
> >
> > On Mon, Nov 20, 2017 at 10:10 AM, Uri Simchoni <[hidden email]> wrote:
> >
> >> On 11/18/2017 01:35 AM, Jeremy Allison via samba-technical wrote:
> >>>
> >>> Right now, can you type:
> >>>
> >>> make test
> >>>
> >>> on a FreeBSD Samba build and have all the tests pass ? I
> >>> would currently doubt it, as many of the tests (like the
> >>> Linux kernel oplock and change notify ones) require Linux
> >>> specific features. Also, some of the tevent tests require
> >>> epoll etc. etc.
> >>>
> >>
> >> As step 0.1, here's a patch set that make Samba build successfully in
> >> developer mode on FreeBSD 11.1-RELEASE (clang 4.0.0). Most of this is
> >> clang-related, so I suppose it has value beyond FreeBSD.
> >>
> >> The configuration command I used was:
> >>
> >> ADDITIONAL_CFLAGS="-Wno-unknown-warning-option" ./configure.developer
> >> --with-selftest-prefix=./bin/ab  --picky-developer --abi-check-disable
> >> --prefix=<my prefix> --with-profiling-data
> >>
> >> This is *almost* the Samba autobuild configuration, except:
> >> 1. No ABI checking - It looks like ABI checking depends on some tool
> >> that emits differently-formatted output on FreeBSD
> >>
> >> 2. The no-unknonwn-warning-option - I couldn't figure out how to make
> >> clang, during configuration time, identify whether it supports a warning
> >> option or not, *without* supplying other command line options which
> >> could bread another compiler. Upstream waf simply identifies clang, so
> >> one might use that to insert clang-specific behavior.
> >>
> >> I'm not sure when (and if...) I'll be able to continue this and see what
> >> are the make test hurdles. If we can hook this to some CI, we can detect
> >> post-push build breakage and fix it before release.
> >>
> >> Thanks,
> >> Uri.
> >>
> >>> This is doable, but not an easy change to make.
> >>>
> >>
> >>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v2] Fix FreeBSD developer build (was: Re: vfs_zfsacl.c: use of undeclared identifier 'ace')

Samba - samba-technical mailing list
On Mon, 2017-11-20 at 23:58 +0100, Timur I. Bakeyev via samba-technical
wrote:

> Hi, Uri!
>
> That's quite a jumbo patch, possibly splitting it into smaller chunks could
> be easier to review :)
>
> I've noticed mentioning of missing HAVE_INTPTR_T on FreeBSD, need to check
> that, but meanwhile:
>
> https://bugzilla.samba.org/show_bug.cgi?id=4635
>
> Also, is that patchset against the HEAD? As traditionally we usually first
> work with releases, so at least two patches fail to me...

Samba patches are generally developed against master, backports are a
secondary task as they need the git hash from master for the cherry-
pick.

On the question of how to keep this working, one option might be to get
Travis CI building Samba on MacOS, that might be close enough to cover
some of the issues.  That doesn't stop patches get into master, but at
least it can be monitored by watching the build status on GitHub / pull
requests.

Thanks,

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: CI beyond sn-devel

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Sun, 2017-11-19 at 11:42 +0200, Alexander Bokovoy via samba-
technical wrote:
>
> I have freeIPA PR CI running also against my personal freeIPA clone on
> github. That PR CI is using my own freeIPA PR CI runner instance on my
> desktop. So every time I push something and propose as a clone to
> myself, I trigger the test runs. Here is an example:
> https://github.com/abbra/freeipa/pull/5.  Scroll down to "All checks
> passed" to see four different scenarious that were executed and links to
> actual logs. The logs are actually pushed to some Fedora hosted server
> which is used as a logs storage for each build.


Note that you have to be logged in github to be able to see these.

Simo.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v2] Fix FreeBSD developer build

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

Attached version 3 - same as v2, but with added 3 patches at the end,
which allow FreeBSD developer build to run without the ADDITIONAL_CFLAGS
workaround, and without breaking Linux...

Some responses below.

Thanks,
Uri.

On 11/21/2017 12:58 AM, Timur I. Bakeyev via samba-technical wrote:
> Hi, Uri!
>
> That's quite a jumbo patch, possibly splitting it into smaller chunks could
> be easier to review :)
>

That's a jumbo patch-*set*. Each patch is pretty small, concerns one
issue, and most patches modify a single file. I believe that's more or
less the Samba practice. Granted, it's easier for people to review a
small patch set, but that would mean I'd get into a cycle of several
weeks, waiting for the previous subset to land before submitting the
next one - I don't have the patience for that so let's keep our fingers
crossed.

> I've noticed mentioning of missing HAVE_INTPTR_T on FreeBSD, need to check
> that, but meanwhile:
>
> https://bugzilla.samba.org/show_bug.cgi?id=4635
>
> Also, is that patchset against the HEAD? As traditionally we usually first
> work with releases, so at least two patches fail to me...
>
> What are the problems with the patches I sent on Linux? I believe similar
> approach is used in couple of other places in wscripts.
>
I didn't want to use the patches as-is, as I don't support the notion
of CHECK_CFLAGS(<flag-we-want-to-check> + <error flags>). CHECK_CFLAGS
should be able to detect <flag-we-want-to-check> without having to add.
So what I did was make a similar change to CHECK_CFLAGS itself. That
worked on FreeBSD, but changed the set of supported flags on Fedora 25
(GCC 6.4.1), all of the sudden claiming that -Wformat-security is not
supported. With further examination, it turned out that
-Wformat-security is ignored when -Wformat is not specified, hence the
addition of "prereq_flags" to CHECK_FLAGS.

> With regards,
> Timur Bakeyev.
>
>
>
>
>
> On Mon, Nov 20, 2017 at 11:00 PM, Uri Simchoni <[hidden email]> wrote:
>
>> Attached v2, with 16/35 modified so as not to change the params.
>>
>> As for the cflag detection tricks - What I tried changed things on the
>> Linux/gcc side, so I propose to leave it at that (with the
>> ADDITIONAL_CFLAGS workaround) and attack this later.
>>
>> Thanks,
>> Uri.
>>
>> On 11/20/2017 03:26 PM, Timur I. Bakeyev via samba-technical wrote:
>>> Thanks, Uri, a great work have been done to chaise all this annoying
>>> warnings. Hope those will end up in the master tree.
>>>
>>> In you "[PATCH 16/35] winbind_nss_freebsd: fix a const discard warning"
>>> though you change the signature of the function:
>>>
>>> int
>>> __getgroupmembership(const char *uname, gid_t agroup, gid_t *groups,
>>>         int maxgrp, int *grpcnt)
>>>
>>> which should be avoided. Need to fix this warning somewhere else.
>>>
>>> As for the Clang being liberal regarding unknown options we use the
>>> following trick:
>>>
>>> --- buildtools/wafsamba/samba_autoconf.py.orig  2017-07-04 10:05:25 UTC
>>> +++ buildtools/wafsamba/samba_autoconf.py
>>> @@ -873,7 +873,7 @@ def SAMBA_CHECK_UNDEFINED_SYMBOL_FLAGS(c
>>>          conf.env.undefined_ldflags = conf.ADD_LDFLAGS('-Wl,-no-
>> undefined',
>>> testflags=True)
>>>
>>>      if not sys.platform.startswith("openbsd") and
>>> conf.env.undefined_ignore_ldflags == []:
>>> -        if conf.CHECK_LDFLAGS(['-undefined', 'dynamic_lookup']):
>>> +        if conf.CHECK_LDFLAGS(['-undefined', 'dynamic_lookup'] +
>>> conf.env.WERROR_CFLAGS):
>>>              conf.env.undefined_ignore_ldflags = ['-undefined',
>>> 'dynamic_lookup']
>>>
>>>  @conf
>>> --- lib/replace/wscript.orig    2017-11-02 11:38:36 UTC
>>> +++ lib/replace/wscript
>>> @@ -81,7 +81,7 @@ def configure(conf):
>>>      conf.CHECK_HEADERS('sys/atomic.h')
>>>      conf.CHECK_HEADERS('libgen.h')
>>>
>>> -    if conf.CHECK_CFLAGS('-Wno-format-truncation'):
>>> +    if conf.CHECK_CFLAGS(['-Wno-format-truncation'] +
>>> conf.env.WERROR_CFLAGS):
>>>          conf.define('HAVE_WNO_FORMAT_TRUNCATION', '1')
>>>
>>>      # Check for process set name support
>>>
>>>
>>> There are a lot of small fixes like this in the FreeBSD Samba port, hope
>>> they'll find their way into master as well.
>>>
>>> With regards,
>>> Timur Bakeyev.
>>>
>>> On Mon, Nov 20, 2017 at 10:10 AM, Uri Simchoni <[hidden email]> wrote:
>>>
>>>> On 11/18/2017 01:35 AM, Jeremy Allison via samba-technical wrote:
>>>>>
>>>>> Right now, can you type:
>>>>>
>>>>> make test
>>>>>
>>>>> on a FreeBSD Samba build and have all the tests pass ? I
>>>>> would currently doubt it, as many of the tests (like the
>>>>> Linux kernel oplock and change notify ones) require Linux
>>>>> specific features. Also, some of the tevent tests require
>>>>> epoll etc. etc.
>>>>>
>>>>
>>>> As step 0.1, here's a patch set that make Samba build successfully in
>>>> developer mode on FreeBSD 11.1-RELEASE (clang 4.0.0). Most of this is
>>>> clang-related, so I suppose it has value beyond FreeBSD.
>>>>
>>>> The configuration command I used was:
>>>>
>>>> ADDITIONAL_CFLAGS="-Wno-unknown-warning-option" ./configure.developer
>>>> --with-selftest-prefix=./bin/ab  --picky-developer --abi-check-disable
>>>> --prefix=<my prefix> --with-profiling-data
>>>>
>>>> This is *almost* the Samba autobuild configuration, except:
>>>> 1. No ABI checking - It looks like ABI checking depends on some tool
>>>> that emits differently-formatted output on FreeBSD
>>>>
>>>> 2. The no-unknonwn-warning-option - I couldn't figure out how to make
>>>> clang, during configuration time, identify whether it supports a warning
>>>> option or not, *without* supplying other command line options which
>>>> could bread another compiler. Upstream waf simply identifies clang, so
>>>> one might use that to insert clang-specific behavior.
>>>>
>>>> I'm not sure when (and if...) I'll be able to continue this and see what
>>>> are the make test hurdles. If we can hook this to some CI, we can detect
>>>> post-push build breakage and fix it before release.
>>>>
>>>> Thanks,
>>>> Uri.
>>>>
>>>>> This is doable, but not an easy change to make.
>>>>>
>>>>
>>>>
>>
>>


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

Re: [PATCHES v2] Fix FreeBSD developer build

Samba - samba-technical mailing list
On Tue, 2017-11-21 at 21:25 +0200, Uri Simchoni via samba-technical
wrote:

> Hi,
>
> Attached version 3 - same as v2, but with added 3 patches at the end,
> which allow FreeBSD developer build to run without the ADDITIONAL_CFLAGS
> workaround, and without breaking Linux...
>
> Some responses below.
>
> Thanks,
> Uri.
>
> On 11/21/2017 12:58 AM, Timur I. Bakeyev via samba-technical wrote:
> > Hi, Uri!
> >
> > That's quite a jumbo patch, possibly splitting it into smaller chunks could
> > be easier to review :)
> >
>
> That's a jumbo patch-*set*. Each patch is pretty small, concerns one
> issue, and most patches modify a single file. I believe that's more or
> less the Samba practice. Granted, it's easier for people to review a
> small patch set, but that would mean I'd get into a cycle of several
> weeks, waiting for the previous subset to land before submitting the
> next one - I don't have the patience for that so let's keep our fingers
> crossed.
I've taken a go at it.  I'll review what I can of this, so as to cut
down the set into the changes I can simply approve.

In particular, I've dropped the pragma changes.  #pragma caused chaos
last time we used it, so I'm skipping those for now.

I'm also skipping the changes to pam_wrapper that need to go upstream
(via Andreas) first.  It looks like I also made the same mistake when
some python3 patches landed, oops.

> > I've noticed mentioning of missing HAVE_INTPTR_T on FreeBSD, need to check
> > that, but meanwhile:
> >
> > https://bugzilla.samba.org/show_bug.cgi?id=4635
> >
> > Also, is that patchset against the HEAD? As traditionally we usually first
> > work with releases, so at least two patches fail to me...
> >
> > What are the problems with the patches I sent on Linux? I believe similar
> > approach is used in couple of other places in wscripts.
> >
>
> I didn't want to use the patches as-is, as I don't support the notion
> of CHECK_CFLAGS(<flag-we-want-to-check> + <error flags>). CHECK_CFLAGS
> should be able to detect <flag-we-want-to-check> without having to add.
> So what I did was make a similar change to CHECK_CFLAGS itself. That
> worked on FreeBSD, but changed the set of supported flags on Fedora 25
> (GCC 6.4.1), all of the sudden claiming that -Wformat-security is not
> supported. With further examination, it turned out that
> -Wformat-security is ignored when -Wformat is not specified, hence the
> addition of "prereq_flags" to CHECK_FLAGS.
I like this approach.

Attached is what I'm happy with, which is under a private autobuild.

This should then allow broader review of the remaining changes, without
the weight of the larger patch set.

Thanks!

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




freebsd-fixes-for-master.patch.txt (56K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v2] Fix FreeBSD developer build

Samba - samba-technical mailing list
On Wed, 2017-11-22 at 13:15 +1300, Andrew Bartlett via samba-technical
wrote:

> On Tue, 2017-11-21 at 21:25 +0200, Uri Simchoni via samba-technical
> wrote:
> > Hi,
> >
> > Attached version 3 - same as v2, but with added 3 patches at the end,
> > which allow FreeBSD developer build to run without the ADDITIONAL_CFLAGS
> > workaround, and without breaking Linux...
> >
> > Some responses below.
> >
> > Thanks,
> > Uri.
> >
> > On 11/21/2017 12:58 AM, Timur I. Bakeyev via samba-technical wrote:
> > > Hi, Uri!
> > >
> > > That's quite a jumbo patch, possibly splitting it into smaller chunks could
> > > be easier to review :)
> > >
> >
> > That's a jumbo patch-*set*. Each patch is pretty small, concerns one
> > issue, and most patches modify a single file. I believe that's more or
> > less the Samba practice. Granted, it's easier for people to review a
> > small patch set, but that would mean I'd get into a cycle of several
> > weeks, waiting for the previous subset to land before submitting the
> > next one - I don't have the patience for that so let's keep our fingers
> > crossed.
>
> I've taken a go at it.  I'll review what I can of this, so as to cut
> down the set into the changes I can simply approve.
>
> In particular, I've dropped the pragma changes.  #pragma caused chaos
> last time we used it, so I'm skipping those for now.

One change that I've reviewed, but I'm not very happy with is this one:

 sysacls: make SMB_ACL_PERMSET_T opaque
    
    sys_acl_get_permset() returns a pointer to a uint32_t. Prior
    to this change, SMB_ACL_PERMSET_T has been defined as a mode_t
    pointer, where mode_t may or may not be uint32_t (on FreeBSD
    it's uint16_t, which could screw up big-endian FreeBSD systems).
    
    Since SMB_ACL_PERMSET_T is being used as an opaque
    data type to the ouside world, this patch changes it
    to a void*, and in the implementation casts the pointer
    to a uint32_t pointer.
    
    Signed-off-by: Uri Simchoni <[hidden email]>
    Reviewed-by: Andrew Bartlett <[hidden email]>

Could you see if you could re-do it to instead change the interface
where the mode_t matters, and fix it as a uint32_t in the type.  void*
is to be avoided if possible.

Can you also explain more how the change in the type hurts things?

If it can't be done, then my review stands, but can you take a look
again?

In the meantime I've pushed the rest of this partial set to autobuild,
hopefully that makes review of the remainder easier.

Thanks,

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





12