WERR_OK vs WERR_SUCCESS (was: Re: [PATCH] add generated NTSTATUS error module for python)
Andrew Bartlett
abartlet at samba.org
Sun Feb 12 09:15:32 UTC 2017
On Sun, 2017-02-12 at 16:11 +1300, Andrew Bartlett wrote:
> 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
> > WERRORs
> > 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_compu
> > te
> > 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,
> 'WERR_OK'):
> python/samba/netcmd/dns.py: if e.args[1] ==
> 'WERR_DNS_ERROR_NAME_DOES_NOT_EXIST':
> python/samba/netcmd/dns.py: if e.args[1] ==
> 'WERR_DNS_ERROR_ZONE_ALREADY_EXISTS':
> python/samba/netcmd/dns.py: if e.args[1] ==
> 'WERR_DNS_ERROR_ZONE_DOES_NOT_EXIST':
> python/samba/netcmd/dns.py: if e.args[1] ==
> 'WERR_DNS_ERROR_NAME_DOES_NOT_EXIST':
> python/samba/netcmd/dns.py: if e.args[1] ==
> 'WERR_DNS_ERROR_NAME_DOES_NOT_EXIST':
> python/samba/netcmd/dns.py: if e.args[1] ==
> 'WERR_DNS_ERROR_NAME_DOES_NOT_EXIST':
> python/samba/netcmd/dns.py: if e.args[1] ==
> 'WERR_DNS_ERROR_NAME_DOES_NOT_EXIST':
> python/samba/netcmd/domain.py: if werr == 8452:
> #WERR_DS_DRA_NO_REPLICA
> python/samba/netcmd/domain.py: if werr == 8452:
> #WERR_DS_DRA_NO_REPLICA
> 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 =
> 0x000006D1
> 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 !=
> self.WERR_OK:
> 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 !=
> self.WERR_OK:
> 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 !=
> self.WERR_OK:
> 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:
> #WERR_DNS_ERROR_NAME_DOES_NOT_EXIST
> python/samba/remove_dc.py: if enum == 0x000025F2:
> #WERR_DNS_ERROR_NAME_DOES_NOT_EXIST
> python/samba/tests/dns.py: if num != 9601:
> #WERR_DNS_ERROR_ZONE_DOES_NOT_EXIST
>
> 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
> history).
>
> 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.
On examination of the NTSTATUS case, we have the same issue.
NT_STATUS_OK is incredibly strongly embedded in Samba, we should
instead have this be an exception in the error tables, and likewise use
WERR_OK for consistency.
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