Success story with patch: Samba4 on FreeBSD - one year in production

[Andriy Syrovenko]

Hi,

We were running Samba4a14 on FreeBSD 8.2 in production for more then a
year, and have recently upgraded to a20 on FreeBSD 9.0. It serves a
small network of about 50 PCs running XP, Vista and 7. Despite being
named 'alpha', Samba4 works really stable- no major issues so far.
Great work, folks!

Please find attached a small patch I prepared against alpha20. This
patch has to be applied to the Samba4's source tree in order to make
Samba4 successfully build and run on FreeBSD. The patch addresses
three issues:

1. Quite a few of Samba's external dependencies are not in the base on
FreeBSD, but rather installed from ports/packages, and thus reside in
/usr/local (/usr/local/include for headers and /usr/local/lib for
shared objects). These are, for instance, gettext, iconv, popt just to
name a few. So I modified 'buildtools/wafsamba/samba_optimisation.py'
to append the required paths to _CCINCFLAGS and LINKFLAGS.

2. There are 'sys/uuid.h' header in /usr/include on FreeBSD, which
defines struct uuid but does not define any functions to work with it.
The e2fsprogs-libuuid package, on the other hand, installs
'uuid/uuid.h' header in /usr/local/include, which defines both struct
uuid as well as related functions. The problem is the definitions of
struct uuid in these files are different and are not compatible with
each other, so I modified #ifdef in 'lib/addns/dnsutils.c' file so
that only 'uuid/uuid.h' header is included in case both header are
present in the system.

3. The third part of the patch adds extattr support on FreeBSD to the
Samba4 binaries. Extended attributes handling on FreeBSD differs from
Linux in that the attribute name does not include the namespace name.
Namespace is specified as an additional (numeric) parameter to the
extattr related function instead. Additionally, there only two
universally defined namespaces- system and user, and there no security
namespace. So, I decided to put everything into the user namespace
using the original attribute names as they are defined in Samba
(including '.' character which seems to be perfectly legal on
FreeBSD).

Please consider the possibility to include this patch into the main
source tree. I can work on some changes if required.



There were also some issues while upgrading a14 to a20 which I'd like to report.

1. There were some locking problems that resulted in the following
error messages during upgradeprovision:

ltdb: tdb(/usr/local/samba/private/sam.ldb): Locking error:
tdb_allrecord_upgrade failed: count 0 too high
ltdb: tdb(/usr/local/samba/private/sam.ldb): Locking error:
tdb1_transaction_prepare_commit: failed to upgrade hash locks
ltdb: tdb(/usr/local/samba/private/sam.ldb.d/CN=SCHEMA,CN=CONFIGURATION,DC=SYTOSS,DC=INTRA.ldb):
Invalid parameter: tdb1_transaction_cancel: no transaction
ltdb: tdb(/usr/local/samba/private/sam.ldb.d/CN=CONFIGURATION,DC=SYTOSS,DC=INTRA.ldb):
Invalid parameter: tdb1_transaction_cancel: no transaction
ltdb: tdb(/usr/local/samba/private/sam.ldb.d/DC=SYTOSS,DC=INTRA.ldb):
Invalid parameter: tdb1_transaction_cancel: no transaction
partition del transaction mismatch

To deal with them I just recreated all LDB files using tdbdump and
tdbrestore prior to upgradeprovision. Hope I did not break anything.
:)

2. Alpha17's readme file suggests running 'samba-tool dbcheck
--cross-ncs --fix'. As it turned out this tool tries to fix some
errors in schema partitions as well, which is not allowed by default,
so the right way to run this tool seems to be:

samba-tool dbcheck --cross-ncs --fix --option="dsdb:schema update allowed"=true

3. I had to add the following line to 'samba_upgradedns', so it could
locate the required modules.

sys.path.insert(0, "/usr/local/samba/lib/python2.7/site-packages")

4. Bind DLZ module requires some libraries that are placed in
/usr/local/samba/lib/private, which does not seem to be in RPATH for
these modules. The libraries in question are 'libsamdb-common.so',
'libldb.so.1', 'libauthkrb5.so', 'libndr-samba.so' and 'libevents.so'.
To fix the problem I created symlinks to these libraries in
/usr/local/samba/lib, though I am not sure it was the right way to go.



Best regards,
Andrey.

[Andrew Bartlett]

On Mon, 2012-05-21 at 14:39 +0300, Andriy Syrovenko wrote:

> Hi,
>
> We were running Samba4a14 on FreeBSD 8.2 in production for more then a
> year, and have recently upgraded to a20 on FreeBSD 9.0. It serves a
> small network of about 50 PCs running XP, Vista and 7. Despite being
> named 'alpha', Samba4 works really stable- no major issues so far.
> Great work, folks!
>
> Please find attached a small patch I prepared against alpha20. This
> patch has to be applied to the Samba4's source tree in order to make
> Samba4 successfully build and run on FreeBSD. The patch addresses
> three issues:

To get the changes into master, we need to make a few changes.  If you
want to take them on, then great, otherwise I'll use it as a TODO list.

> 1. Quite a few of Samba's external dependencies are not in the base on
> FreeBSD, but rather installed from ports/packages, and thus reside in
> /usr/local (/usr/local/include for headers and /usr/local/lib for
> shared objects). These are, for instance, gettext, iconv, popt just to
> name a few. So I modified 'buildtools/wafsamba/samba_optimisation.py'
> to append the required paths to _CCINCFLAGS and LINKFLAGS.

I've started work on this.  See how in master --with-libiconv now takes
an argument and defaults to /usr/local?  This we need to do for all the
other packages you list.

> 2. There are 'sys/uuid.h' header in /usr/include on FreeBSD, which
> defines struct uuid but does not define any functions to work with it.
> The e2fsprogs-libuuid package, on the other hand, installs
> 'uuid/uuid.h' header in /usr/local/include, which defines both struct
> uuid as well as related functions. The problem is the definitions of
> struct uuid in these files are different and are not compatible with
> each other, so I modified #ifdef in 'lib/addns/dnsutils.c' file so
> that only 'uuid/uuid.h' header is included in case both header are
> present in the system.

I've looked at the uuid use, and I can't see any good excuse for this
external dependency in this code.  We have our own GUID handlers,
including GUID_random() and GUID_string().  The adddns code should just
use that.

> 3. The third part of the patch adds extattr support on FreeBSD to the
> Samba4 binaries. Extended attributes handling on FreeBSD differs from
> Linux in that the attribute name does not include the namespace name.
> Namespace is specified as an additional (numeric) parameter to the
> extattr related function instead. Additionally, there only two
> universally defined namespaces- system and user, and there no security
> namespace. So, I decided to put everything into the user namespace
> using the original attribute names as they are defined in Samba
> (including '.' character which seems to be perfectly legal on
> FreeBSD).

The correct way to handle this is to move the code in
source3/lib/system.c to lib/replace, changing it from sys_fgetxattr to
rep_fgetxattr(), and then adding the replacement macros.

> Please consider the possibility to include this patch into the main
> source tree. I can work on some changes if required.

I would very much appreciate it if you could, but understand if it's too
much.

> There were also some issues while upgrading a14 to a20 which I'd like to report.
>
> 1. There were some locking problems that resulted in the following
> error messages during upgradeprovision:
>
> ltdb: tdb(/usr/local/samba/private/sam.ldb): Locking error:
> tdb_allrecord_upgrade failed: count 0 too high
> ltdb: tdb(/usr/local/samba/private/sam.ldb): Locking error:
> tdb1_transaction_prepare_commit: failed to upgrade hash locks
> ltdb: tdb(/usr/local/samba/private/sam.ldb.d/CN=SCHEMA,CN=CONFIGURATION,DC=SYTOSS,DC=INTRA.ldb):
> Invalid parameter: tdb1_transaction_cancel: no transaction
> ltdb: tdb(/usr/local/samba/private/sam.ldb.d/CN=CONFIGURATION,DC=SYTOSS,DC=INTRA.ldb):
> Invalid parameter: tdb1_transaction_cancel: no transaction
> ltdb: tdb(/usr/local/samba/private/sam.ldb.d/DC=SYTOSS,DC=INTRA.ldb):
> Invalid parameter: tdb1_transaction_cancel: no transaction
> partition del transaction mismatch
>
> To deal with them I just recreated all LDB files using tdbdump and
> tdbrestore prior to upgradeprovision. Hope I did not break anything.
> :)

That's odd.  I'll leave this for a tdb maintainer to comment on.

> 2. Alpha17's readme file suggests running 'samba-tool dbcheck
> --cross-ncs --fix'. As it turned out this tool tries to fix some
> errors in schema partitions as well, which is not allowed by default,
> so the right way to run this tool seems to be:
>
> samba-tool dbcheck --cross-ncs --fix --option="dsdb:schema update allowed"=true

Thanks, we will need to ensure that the schema update dis-allowance code
considers the dbwrap control

> 3. I had to add the following line to 'samba_upgradedns', so it could
> locate the required modules.
>
> sys.path.insert(0, "/usr/local/samba/lib/python2.7/site-packages")

I think this has been fixed - because in the original source used "
rather than ' quotes, it wasn't replaced during make install.

> 4. Bind DLZ module requires some libraries that are placed in
> /usr/local/samba/lib/private, which does not seem to be in RPATH for
> these modules. The libraries in question are 'libsamdb-common.so',
> 'libldb.so.1', 'libauthkrb5.so', 'libndr-samba.so' and 'libevents.so'.
> To fix the problem I created symlinks to these libraries in
> /usr/local/samba/lib, though I am not sure it was the right way to go.

I understand this has since been fixed.

Thank you very much for the feedback, and do let me know if you want to
take on any of the changes.

Thanks!

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org

[Andriy Syrovenko]

Hi Andrew,

Thank you for your response.

>> Please consider the possibility to include this patch into the main
>> source tree. I can work on some changes if required.
>
> I would very much appreciate it if you could, but understand if it's too
> much.

I'm interested in Samba4 supporting FreeBSD out of the box, so I'll
try to find some time to work on it. I think I should start from the
following:

>> 1. Quite a few of Samba's external dependencies are not in the base on
>> FreeBSD, but rather installed from ports/packages, and thus reside in
>> /usr/local (/usr/local/include for headers and /usr/local/lib for
>> shared objects). These are, for instance, gettext, iconv, popt just to
>> name a few. So I modified 'buildtools/wafsamba/samba_optimisation.py'
>> to append the required paths to _CCINCFLAGS and LINKFLAGS.
>
> I've started work on this.  See how in master --with-libiconv now takes
> an argument and defaults to /usr/local?  This we need to do for all the
> other packages you list.

I'm fairly certain I will have some questions, so the question is
should I send them to the list or to someone in person?

Regards,
Andrey.

[Andrew Bartlett]

On Tue, 2012-05-22 at 13:29 +0300, Andriy Syrovenko wrote:

> Hi Andrew,
>
> Thank you for your response.
>
> >> Please consider the possibility to include this patch into the main
> >> source tree. I can work on some changes if required.
> >
> > I would very much appreciate it if you could, but understand if it's too
> > much.
>
> I'm interested in Samba4 supporting FreeBSD out of the box, so I'll
> try to find some time to work on it. I think I should start from the
> following:
>
> >> 1. Quite a few of Samba's external dependencies are not in the base on
> >> FreeBSD, but rather installed from ports/packages, and thus reside in
> >> /usr/local (/usr/local/include for headers and /usr/local/lib for
> >> shared objects). These are, for instance, gettext, iconv, popt just to
> >> name a few. So I modified 'buildtools/wafsamba/samba_optimisation.py'
> >> to append the required paths to _CCINCFLAGS and LINKFLAGS.
> >
> > I've started work on this.  See how in master --with-libiconv now takes
> > an argument and defaults to /usr/local?  This we need to do for all the
> > other packages you list.
>
> I'm fairly certain I will have some questions, so the question is
> should I send them to the list or to someone in person?

Either is fine, as is IRC: https://www.samba.org/samba/irc.html

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org

[Andriy Syrovenko]

Hi,

Please find attached the first part of the revised patch. It addresses
the issue with some dependencies being installed in /usr/local and
eliminates the dependency on the external uuid library.

It turned out that from all the external dependencies, location of
only two packages needs to be specified explicitly. These are iconv
and gettext, so I added the '--with-gettext' option (in addition to
the already existing '--with-libiconv') and defaulted it to
/usr/local. Other packages are being found through pkg-config, except
openldap-client which anyway magically gets located in /usr/local (and
I don't really understand how it's done).

If this patch is acceptable, then I'll look into the remaining part
which is moving the extattr-related code in
source3/lib/system.c to lib/replace.

Best regards,
Andrey.

2012/5/22 Andriy Syrovenko <[hidden email]>:

> Hi Andrew,
>
> Thank you for your response.
>
>>> Please consider the possibility to include this patch into the main
>>> source tree. I can work on some changes if required.
>>
>> I would very much appreciate it if you could, but understand if it's too
>> much.
>
> I'm interested in Samba4 supporting FreeBSD out of the box, so I'll
> try to find some time to work on it. I think I should start from the
> following:
>
>>> 1. Quite a few of Samba's external dependencies are not in the base on
>>> FreeBSD, but rather installed from ports/packages, and thus reside in
>>> /usr/local (/usr/local/include for headers and /usr/local/lib for
>>> shared objects). These are, for instance, gettext, iconv, popt just to
>>> name a few. So I modified 'buildtools/wafsamba/samba_optimisation.py'
>>> to append the required paths to _CCINCFLAGS and LINKFLAGS.
>>
>> I've started work on this.  See how in master --with-libiconv now takes
>> an argument and defaults to /usr/local?  This we need to do for all the
>> other packages you list.
>
> I'm fairly certain I will have some questions, so the question is
> should I send them to the list or to someone in person?
>
> Regards,
> Andrey.

[Andrew Bartlett]

On Thu, 2012-05-24 at 20:01 +0300, Andriy Syrovenko wrote:

> Hi,
>
> Please find attached the first part of the revised patch. It addresses
> the issue with some dependencies being installed in /usr/local and
> eliminates the dependency on the external uuid library.
>
> It turned out that from all the external dependencies, location of
> only two packages needs to be specified explicitly. These are iconv
> and gettext, so I added the '--with-gettext' option (in addition to
> the already existing '--with-libiconv') and defaulted it to
> /usr/local. Other packages are being found through pkg-config, except
> openldap-client which anyway magically gets located in /usr/local (and
> I don't really understand how it's done).
>
> If this patch is acceptable, then I'll look into the remaining part
> which is moving the extattr-related code in
> source3/lib/system.c to lib/replace.

That patch looks great!

To give you proper credit, could you put them into individual patches,
commit them into git and then export them with (say) 'git-format-patch
-3'?

https://wiki.samba.org/index.php/Using_Git_for_Samba_Development has
some hints.

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org

[Andriy Syrovenko]

Done. New patches are in the attached files.

Regards,
Andrey.

2012/5/25 Andrew Bartlett <[hidden email]>:
> That patch looks great!
>
> To give you proper credit, could you put them into individual patches,
> commit them into git and then export them with (say) 'git-format-patch
> -3'?

[Andrew Bartlett]

On Mon, 2012-05-21 at 14:39 +0300, Andriy Syrovenko wrote:
> Hi,

> 3. The third part of the patch adds extattr support on FreeBSD to the
> Samba4 binaries. Extended attributes handling on FreeBSD differs from
> Linux in that the attribute name does not include the namespace name.
> Namespace is specified as an additional (numeric) parameter to the
> extattr related function instead. Additionally, there only two
> universally defined namespaces- system and user, and there no security
> namespace. So, I decided to put everything into the user namespace
> using the original attribute names as they are defined in Samba
> (including '.' character which seems to be perfectly legal on
> FreeBSD).
>
> Please consider the possibility to include this patch into the main
> source tree. I can work on some changes if required.

Just a quick note to say that I've taken on this larger challenge, and
hopefully we will have patches for review soon.

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org

[Andriy Syrovenko]

Hi Andrew,

Thank you for the note. I was planning to work on these patches during
the upcoming weekend, but since you've already started the work I'd
better spend this time testing you patches.

Regards,
Andrey.

2012/6/1 Andrew Bartlett <[hidden email]>:

> On Mon, 2012-05-21 at 14:39 +0300, Andriy Syrovenko wrote:
>> Hi,
>
>> 3. The third part of the patch adds extattr support on FreeBSD to the
>> Samba4 binaries. Extended attributes handling on FreeBSD differs from
>> Linux in that the attribute name does not include the namespace name.
>> Namespace is specified as an additional (numeric) parameter to the
>> extattr related function instead. Additionally, there only two
>> universally defined namespaces- system and user, and there no security
>> namespace. So, I decided to put everything into the user namespace
>> using the original attribute names as they are defined in Samba
>> (including '.' character which seems to be perfectly legal on
>> FreeBSD).
>>
>> Please consider the possibility to include this patch into the main
>> source tree. I can work on some changes if required.
>
> Just a quick note to say that I've taken on this larger challenge, and
> hopefully we will have patches for review soon.
>
> Andrew Bartlett
>
> --
> Andrew Bartlett                                http://samba.org/~abartlet/
> Authentication Developer, Samba Team           http://samba.org
>

[Andrew Bartlett]

On Fri, 2012-06-01 at 14:17 +0300, Andriy Syrovenko wrote:
> Hi Andrew,
>
> Thank you for the note. I was planning to work on these patches during
> the upcoming weekend, but since you've already started the work I'd
> better spend this time testing you patches.

Thanks.  I've been iterating over this over the past day, and a further
fixed version should be in master in an hour.

There is an issue with the autoconf build, not for FreeBSD, but for
other platforms with xattrs in -lattr.  I'll need some help from an
autoconf build expert to solve that one.

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org