WERR_OK vs WERR_SUCCESS (was: Re: [PATCH] add generated NTSTATUS error module for python)

Andrew Bartlett abartlet at samba.org
Sun Feb 12 03:11:08 UTC 2017

On Sun, 2017-02-12 at 09:13 +1300, Andrew Bartlett wrote:
> On Wed, 2017-02-01 at 15:15 +1300, Bob Campbell wrote:
> > Hi again,
> > 
> > Attached are some patches that build on top of the NTSTATUS patches
> > to
> > also generate WERROR codes and descriptions, as well as a similar
> > script
> > to the one attached with the NTSTATUS patches to make sure that we
> > aren't missing any.
> > 
> > Please review and push if appropriate, once the NTSTATUS patches
> > (currently in autobuild) have landed.
> Sadly this fails autobuild, so I did a bisect with:
> ./script/bisect-test.py --good=origin/master --bad=gen-error --test-
> command='make test FAIL_IMMEDIATELY=1 TESTS=acl.py'
> I've pushed the gen-error branch I've used to my repo.
> 3c24ff1d189b7eade045591975d361d13e59b6c6 is the first bad commit
> commit 3c24ff1d189b7eade045591975d361d13e59b6c6
> Author: Bob Campbell <bobcampbell at catalyst.net.nz>
> Date:   Fri Jan 20 12:24:53 2017 +1300
>     errors: add WERROR generation to build system
>     Parts of doserr.c and werror.h are now generated into
> werror_gen.c
> and
>     werror_gen.h, respectively. Also, py_werror.c is now generated.
>     Some errors were not included in the list which we now generate
>     from. These errors have been manually included.
>     Signed-off-by: Bob Campbell <bobcampbell at catalyst.net.nz>
>     Reviewed-by: Andrew Bartlett <abartlet at samba.org>
>     Reviewed-by: Garming Sam <garming at catalyst.net.nz>
> Fails with:
> UNEXPECTED(error):
> samba4.ldap.acl.python(ad_dc_ntvfs).__main__.AclSPNTests.test_compute
> r_
> spn(ad_dc_ntvfs)
> REASON: Exception: Exception: Traceback (most recent call last):
>   File "/data/samba/git/samba/source4/dsdb/tests/python/acl.py", line
> 1792, in setUp
>     self.create_dc(self.dcctx)
>   File "/data/samba/git/samba/source4/dsdb/tests/python/acl.py", line
> 1870, in create_dc
>     ctx.join_add_objects()
>   File "bin/python/samba/join.py", line 614, in join_add_objects
>     ctx.join_add_ntdsdsa()
>   File "bin/python/samba/join.py", line 545, in join_add_ntdsdsa
>     ctx.DsAddEntry([rec])
>   File "bin/python/samba/join.py", line 495, in DsAddEntry
>     ctr.err_data.info.extended_err))
> AttributeError: 'NoneType' object has no attribute 'extended_err'
> Can you please look at this?

This was bugging me, so I looked into what we need to do:

The issue is that this change changed the string from of 0 from WERR_OK
to WERR_SUCCESS.  You tried to cater to that by leaving WERR_OK in the
table in doserr.c, but the generated table applies first.

We need to ensure that entries in the combined table are unique, that
the final table matches the original one (other than odering), and have
a pre-commit that renames WERR_OK to WERR_SUCCESS.

Then we need to get out of the game of using strings and embedded
constants in the python code.  Here is the list that needs to be fixed:

python/samba/join.py:            if ctr.extended_err != (0, 'WERR_OK'):
python/samba/join.py:            if ctr.err_data.status != (0,
python/samba/netcmd/dns.py:        if e.args[1] ==
python/samba/netcmd/dns.py:            if e.args[1] ==
python/samba/netcmd/dns.py:            if e.args[1] ==
python/samba/netcmd/dns.py:            if e.args[1] ==
python/samba/netcmd/dns.py:            if e.args[1] ==
python/samba/netcmd/dns.py:            if e.args[1] ==
python/samba/netcmd/dns.py:            if e.args[1] ==
python/samba/netcmd/domain.py:                    if werr == 8452:
python/samba/netcmd/domain.py:            if werr == 8452:
python/samba/netcmd/domain.py:    WERR_OK = 0x00000000
python/samba/netcmd/domain.py:    WERR_INVALID_FUNCTION = 0x00000001
python/samba/netcmd/domain.py:    WERR_NERR_ACFNOTLOADED = 0x000008B3
python/samba/netcmd/domain.py:    WERR_RPC_S_PROCNUM_OUT_OF_RANGE =
python/samba/netcmd/domain.py:            if
self.check_runtime_error(error, self.WERR_RPC_S_PROCNUM_OUT_OF_RANGE):
python/samba/netcmd/domain.py:                if local_trust_status !=
self.WERR_OK or local_conn_status != self.WERR_OK:
python/samba/netcmd/domain.py:                    if
remote_trust_status != self.WERR_OK or remote_conn_status !=
python/samba/netcmd/domain.py:        if local_trust_status !=
self.WERR_OK or local_conn_status != self.WERR_OK:
python/samba/netcmd/domain.py:        if local_conn_status !=
python/samba/netcmd/domain.py:            if remote_trust_status !=
self.WERR_OK or remote_conn_status != self.WERR_OK:
python/samba/netcmd/domain.py:            if remote_conn_status !=
python/samba/netcmd/domain.py:                if
self.check_runtime_error(error, self.WERR_RPC_S_PROCNUM_OUT_OF_RANGE):
python/samba/netcmd/domain.py:                if
self.check_runtime_error(error, self.WERR_INVALID_FUNCTION):
python/samba/netcmd/domain.py:                if
self.check_runtime_error(error, self.WERR_NERR_ACFNOTLOADED):
python/samba/remove_dc.py:        if enum == 0x000025F2:
python/samba/remove_dc.py:            if enum == 0x000025F2:
python/samba/tests/dns.py:            if num != 9601:

I realise this is a pile more work,  this technical debt has built up
over quite some time.  Thankfully others have been working on the
problem, (see the commits by Günther and Kamen in the doserr.c

I realise WERR_SUCCESS is not a succinct but thankfully we don't use
windows error codes as often, so I think WERR_OK it can go the way of
NT_STATUS_NOPROBLEMO, but if there are objections we can keep it as a C
-only #define. 


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

More information about the samba-technical mailing list