[PATCH] samba-tool: make 'samba-tool user create' work like ADUC

Rowland Penny repenny241155 at gmail.com
Thu Jun 25 06:00:22 MDT 2015

On 25/06/15 12:35, Alexander Bokovoy wrote:
> On Thu, Jun 25, 2015 at 11:29:45AM +0100, Rowland Penny wrote:
>>>> to use, what is its gidNumber and they finally type  ' --gid-number=10000'
>>>> on the end. They then press 'Enter' and create the user, they then go back
>>>> and update their records with the uidNumber they just used.
>>>> Would you like to explain how this is different from the way my patch works
>>>> ?
>>> You patch ignores ID allocation schemas that are configured on Samba AD
>>> DC with Winbindd, completely. We need to have a single place where logic
>>> to allocate IDs is done and that is winbindd. Do not add other logic,
>>> please, use the one that is there already.
>> Taking a member server as an example, windbind can be setup to work in at
>> least two ways, the 'ad' and 'rid' back ends. The 'rid' backend maps the
>> users AD RID to a number, this number should be the same on all member
>> servers, but not on a DC (at the moment). The 'ad' backend has nothing to do
>> with winbind apart from the fact winbind extracts the info from AD.
>> I am *not* adding logic, I am just using what is already there, but in a
>> different way. If you come right down to it, the two main ways of using
>> windbind do not allocate IDs, they are either mapped or extracted.
> You are increasing number of code paths that have logic to create IDs.
> We have that logic managed in idmap backens already so please use that.
> If backend returns you (uint32_t)-1 value, it means SID is not mapped or
> could not be mapped automatically so you would be able to tell that to
> the admin. However, incrementing on your own represents just one
> specific way of allocating the ID. I would rather ask the admin to
> confirm it.
> Additionally, Samba AD DC has idmap module that allocates
> uidNumber/gidNumber, see idmap_sid_to_xid() in source4/winbind/idmap.c.
> At the very least you want to be compatible with it.

I am getting lost here, I am updating a *python* script here,
I am not touching the actual samba c code in any way :-\

> Also, the code
> +        idmax = 0
> +        res = self.search(domain_dn,
> +                          scope=ldb.SCOPE_SUBTREE,
> +                          expression=idexpr,
> +                          attrs=[idattr])
> +        for msg in res:
> +            if idattr in msg:
> +                n = str("%s" % msg[idattr])
> +                idn = int(n)
> +                if idn > idmax:
> +                    idmax = idn
> is wrong -- it will pull either whole user database or just a page (1000
> entries or so) depending on how samdb is setup.

OK, that I can understand, forgot about the page size :-[

>>>>> This is what I meant by a heritage that we would need to deal with
>>>>> later.
>>>>> Also, I don't like the mess that is created by combining formatting
>>>>> changes with functional ones. It is going to beat us in future when any
>>>>> specific bug arises and we'll be doing searches for the offending
>>>>> commit. The way we deal with it is that formatting commits are separate
>>>> >from functional changes.
>>>> Could you explain what you mean by 'formatting' and 'functional' , I think I
>>>> understand the later, but do not have a clue about the former.
>>> More than half of your patch is reformatting existing code rather than
>>> adding new feature. Split that into a separate patch, rather than
>>> combining them together.
>> OK, I will try and split the patch up, but quite a lot of what I altered was
>> because it wouldn't work unless I did alter it i.e one depends on the other.
> If you need to reformat, do it first, then add your changes to the
> formatted code, then do nisadd part. There is no requirement that your
> changes have to be in the single patch.

OK, I will look again at my code.


More information about the samba-technical mailing list