Fwd: [PATCHES] simple gpo patches fixes

[David Mulder]

On 04/18/2018 04:01 PM, Douglas Bagnall via samba-technical wrote:
> On 19/04/18 00:25, David Mulder wrote:
>>
>> On 17.04.2018 17:20, Douglas Bagnall wrote:
>>> On 14/04/18 02:19, David Mulder via samba-technical wrote:
>>>> I've added that type check, as well as couple other checks. I also
>>>> wasn't checking the ads type input or verifying the list and it's length
>>>> before popping from it. Latest sources are on the github pull request.
>>> Thanks. The ADS check is definitely necessary.
>>>
>>> I thought the list was OK without the checks (in that you'd get a Python
>>> exception from PyList_GetItem(), not a segfault), but I see that you're
>>> accepting an empty list, so you do need more checks.
>>>
>>> Two questions:
>>>
>>> 1. Is there a test for the empty list?
>> Sure, if the list is len == 0, it returns success (since there is
>> nothing to refresh).
>>         gp_list_len = PyList_Size(gpo_list);
>>         if (gp_list_len == 0) {
>>                 ret = Py_True;
>>                 goto out;
>>         }
> Yeah, I saw that. I meant a python test that calls this with an empty list.
> If there was it would have failed until now, right?
>
> Now I'm noticing there are no tests for gpo.check_refresh_gpo_list() at all,
> which there should be.
Yes, there is no test. I just tried adding one, but
check_refresh_gpo_list() dies when trying to call cli_full_connection(),
which is odd. Dies in the test env, but works fine outside. I'll post
later when I get this working.
>
>>> 2. If you're only ever looking at the first item, why is it a list?
>> Because under the hood GROUP_POLICY_OBJECT is a linked list, but
>> py_ads_get_gpo_list() iterates over that linked list and turns it into a
>> python list (each element in the python list still holds it's *next
>> pointer, despite now being contained within a python list). In the C
>> code we need the linked list back to operate on, so we grab the first
>> item from the python list, which is the head of our C linked list. The
>> order of a python list is persistent, so we don't need to worry about
>> loosing the head of our linked list. The only time we'd end up operating
>> on something other than the head of our linked list, is if someone, say,
>> passed in a splice of the gpo_list, such as
>> gpo.check_refresh_gpo_list(gpo_list[1:]), in which case we'd take the
>> second item in the linked list as the head of our list. This will still
>> work as intended (since any member of the linked list can be the head),
>> and if someone splices the list this way, then that's probably what they
>> intended to happen anyway (this explicitly says to refresh all the gpos
>> expect the first, which makes sense).
> I see problems with that. While this works as intended:
>
> gpo.check_refresh_gpo_list(gpo_list[1:])
>
> if I am following, none of the following will work:
>
> gpo.check_refresh_gpo_list(gpo_list[:2])  # truncate the list
> gpo.check_refresh_gpo_list(gpo_list[:1] + gpo_list[3:])  # remove one
> del gpo_list[2]; gpo.check_refresh_gpo_list(gpo_list)  # remove one
> gpo_list.sort(); gpo.check_refresh_gpo_list(gpo_list)  # reorder
> gpo.check_refresh_gpo_list(gpo_list1 + gpo_list 2)  # join
>
> It would be *slightly* better to use an immutable tuple rather than a list, 
> and better still to have an opaque object from python's point of view. The
> real solution would be to iterate through and reconstruct the linked list
> from the Python list.
Ok, I've fixed this now.
>
> cheers,
> Douglas
>
>

-- 
David Mulder
SUSE Labs Software Engineer - Samba
dmulder at suse.com
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)