[RESEND PATCH] DsBind and DsGetDomainControllerInfo to return 2k8 structs complete

[Garming Sam]

Hi,

I see what you mean now, so that all seems fine. If you haven't already
found it, you might want to look at the definition of drs_DsBind in
python/samba/drs_utils.py for the tests.

Cheers.

On 19/04/16 17:08, voidswitch wrote:
> Hi,
>
> On 19.04.2016 00:56, Garming Sam wrote:
>> Hi Dirk,
>>
>> Just from looking a the patch, I've noticed that there was a info24 that
>> used to be handled as well as the info28 (so a case statement with the
>> three would be preferable). I believe there are some other lengths also
>> available (probably for 2012), but we probably don't handle them
>> properly in other places either so you probably don't need to worry
>> about those (unless you really want to go out of your way).
> drsuapi_bind_state used to store info28 structs only, so any other
> size needed to be converted. This was done only with info24 requests.
> I changed drsuapi_bind_state to use DsBindInfoCtr. Whatever the client
> sends, we simply can store it in drsuapi_bind_state. Same with our
> responses.
>
> Our response should probably take the current functional level into
> account. Currently I send a response which clients should understand,
> as length of the request >= length of the response. Clients sending 24
> requests have always gotten a 28 response.
>>
>> More importantly though, a test would be good to assert that the changes
>> haven't broken anything (and test your new behaviour). A good way to do
>> this would be to order the patches so that first there is the test which
>> passes against the existing levels and fails against the new one. And
>> then would be the patch that fixes the behaviour for the new case so all
>> the tests now pass.
>>
>> An example of how you might start:
>> source4/torture/drs/python/getnc_exop.py
>>
>> You'd need to do the bind at the different levels and basically just
>> assert that the result values are how you would expect. It should be a
>> straightforward test, but a valuable one.
> This should push me in the right direction regarding tests. I'll add
> them.
>>
>> Some minor things:
>> * There's a few C++ '//' comments, which we try to avoid.
>> * The for loop is a little inconsistent, there's spaces for i <
>> res->count, but not for i = 0. And I now realize that it was copied from
>> above... It would be nice to be able to share some part of this code if
>> possible.
> The copy was the simplest option available, since it is really the
> same. I guess a patch to clean this up should follow. Anyway I'll look
> at it.
>>
>> If you haven't already read the README.Coding, it's a good place to
>> start. Otherwise, the changes seem quite reasonable and thanks for
>> working on it. If you need any other help, feel free to ask.
>
> Thanks for the comments,
> Dirk
>
>>
>> Cheers,
>>
>> Garming
>>
>> On 19/04/16 09:20, Void wrote:
>>> I'll get this right eventually.
>>>
>>> Thanks
>>> Dirk
>