[PATCHES v2] Fix GPO unapply log

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCHES v2] Fix GPO unapply log

Samba - samba-technical mailing list
Fixes for 2 bugs in the gpo unapply.
* The apply log was being left empty. Apparently pointers to tree
elements can't be stored in an object, they somehow are disconnected
from the main tree and nothing gets logged. These have to be found each
time we want to add something. This is probably a bug in python ElementTree.
* We should only commit the earliest change from a gpo to the log.
Otherwise we overwrite the original value written by that gpo, and then
we tattoo the setting on unapply.
* Adds testing so this isn't missed again.

 python/samba/gpclass.py               |  67 ++++++++++++++++++++----------------
 source4/scripting/bin/samba_gpoupdate |   4 +--
 source4/torture/gpo/apply.c           | 108 +++++++++++++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 130 insertions(+), 49 deletions(-)

--
David Mulder
SUSE Labs Software Engineer - Samba
[hidden email]
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)


fix_unapply.mbox (23K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v2] Fix GPO unapply log

Samba - samba-technical mailing list
Hi David,

would it be possible that you attach patches
as test/plain instead of application/mbox?
That would make it much easier to review the patches (at least for me)

Can you please make use of existing helper functions in
source4/torture/gpo/apply.c instead of introducing find_attr_val()
I guess ldb_msg_find_attr_as_string() can be used instead.

Using val = ldb_msg_find_ldb_val() and then
(char*)val->data should be also be replaced by
ldb_msg_find_attr_as_string().

ldb_msg_find_attr_as_[u]int*() together with torture_assert_int_equal()
should be used instead of using atoi() and torture_assert().

Please let someone else review the gpclass.py I don't have time to
wrap my head around this currently.

Thanks!
metze

Am 06.12.2017 um 19:23 schrieb David Mulder via samba-technical:

> Fixes for 2 bugs in the gpo unapply.
> * The apply log was being left empty. Apparently pointers to tree
> elements can't be stored in an object, they somehow are disconnected
> from the main tree and nothing gets logged. These have to be found each
> time we want to add something. This is probably a bug in python ElementTree.
> * We should only commit the earliest change from a gpo to the log.
> Otherwise we overwrite the original value written by that gpo, and then
> we tattoo the setting on unapply.
> * Adds testing so this isn't missed again.
>
>  python/samba/gpclass.py               |  67 ++++++++++++++++++++----------------
>  source4/scripting/bin/samba_gpoupdate |   4 +--
>  source4/torture/gpo/apply.c           | 108 +++++++++++++++++++++++++++++++++++++++++++++++++----------
>  3 files changed, 130 insertions(+), 49 deletions(-)
>


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v2] Fix GPO unapply log

Samba - samba-technical mailing list
Hi,

I'm fine with the changes in gpclass.py. I've also checked that all the
tests pass.

In find_attr_val, you declared a variable in the for loop, which we
don't allow right now. But as metze has said, you should just replace
this function with the existing helper methods in ldb.


Cheers,

Garming

On 07/12/17 19:57, Stefan Metzmacher via samba-technical wrote:

> Hi David,
>
> would it be possible that you attach patches
> as test/plain instead of application/mbox?
> That would make it much easier to review the patches (at least for me)
>
> Can you please make use of existing helper functions in
> source4/torture/gpo/apply.c instead of introducing find_attr_val()
> I guess ldb_msg_find_attr_as_string() can be used instead.
>
> Using val = ldb_msg_find_ldb_val() and then
> (char*)val->data should be also be replaced by
> ldb_msg_find_attr_as_string().
>
> ldb_msg_find_attr_as_[u]int*() together with torture_assert_int_equal()
> should be used instead of using atoi() and torture_assert().
>
> Please let someone else review the gpclass.py I don't have time to
> wrap my head around this currently.
>
> Thanks!
> metze
>
> Am 06.12.2017 um 19:23 schrieb David Mulder via samba-technical:
>> Fixes for 2 bugs in the gpo unapply.
>> * The apply log was being left empty. Apparently pointers to tree
>> elements can't be stored in an object, they somehow are disconnected
>> from the main tree and nothing gets logged. These have to be found each
>> time we want to add something. This is probably a bug in python ElementTree.
>> * We should only commit the earliest change from a gpo to the log.
>> Otherwise we overwrite the original value written by that gpo, and then
>> we tattoo the setting on unapply.
>> * Adds testing so this isn't missed again.
>>
>>  python/samba/gpclass.py               |  67 ++++++++++++++++++++----------------
>>  source4/scripting/bin/samba_gpoupdate |   4 +--
>>  source4/torture/gpo/apply.c           | 108 +++++++++++++++++++++++++++++++++++++++++++++++++----------
>>  3 files changed, 130 insertions(+), 49 deletions(-)
>>
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v2] Fix GPO unapply log

Samba - samba-technical mailing list
OK. I'll implement the suggestions here and post a follow up set of patches.
metze... I typically use mbox because I can just do git am mbox_file.mbox
I don't know what a test/plain file means.

On 12/07/2017 01:57 PM, Garming Sam via samba-technical wrote:

> Hi,
>
> I'm fine with the changes in gpclass.py. I've also checked that all the
> tests pass.
>
> In find_attr_val, you declared a variable in the for loop, which we
> don't allow right now. But as metze has said, you should just replace
> this function with the existing helper methods in ldb.
>
>
> Cheers,
>
> Garming
>
> On 07/12/17 19:57, Stefan Metzmacher via samba-technical wrote:
>> Hi David,
>>
>> would it be possible that you attach patches
>> as test/plain instead of application/mbox?
>> That would make it much easier to review the patches (at least for me)
>>
>> Can you please make use of existing helper functions in
>> source4/torture/gpo/apply.c instead of introducing find_attr_val()
>> I guess ldb_msg_find_attr_as_string() can be used instead.
>>
>> Using val = ldb_msg_find_ldb_val() and then
>> (char*)val->data should be also be replaced by
>> ldb_msg_find_attr_as_string().
>>
>> ldb_msg_find_attr_as_[u]int*() together with torture_assert_int_equal()
>> should be used instead of using atoi() and torture_assert().
>>
>> Please let someone else review the gpclass.py I don't have time to
>> wrap my head around this currently.
>>
>> Thanks!
>> metze
>>
>> Am 06.12.2017 um 19:23 schrieb David Mulder via samba-technical:
>>> Fixes for 2 bugs in the gpo unapply.
>>> * The apply log was being left empty. Apparently pointers to tree
>>> elements can't be stored in an object, they somehow are disconnected
>>> from the main tree and nothing gets logged. These have to be found each
>>> time we want to add something. This is probably a bug in python ElementTree.
>>> * We should only commit the earliest change from a gpo to the log.
>>> Otherwise we overwrite the original value written by that gpo, and then
>>> we tattoo the setting on unapply.
>>> * Adds testing so this isn't missed again.
>>>
>>>  python/samba/gpclass.py               |  67 ++++++++++++++++++++----------------
>>>  source4/scripting/bin/samba_gpoupdate |   4 +--
>>>  source4/torture/gpo/apply.c           | 108 +++++++++++++++++++++++++++++++++++++++++++++++++----------
>>>  3 files changed, 130 insertions(+), 49 deletions(-)
>>>
>
>

--
David Mulder
SUSE Labs Software Engineer - Samba
[hidden email]
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)


Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v2] Fix GPO unapply log

Samba - samba-technical mailing list
He meant text/plain file attachment format.

On 12/8/2017 9:16 AM, David Mulder via samba-technical wrote:

> OK. I'll implement the suggestions here and post a follow up set of patches.
> metze... I typically use mbox because I can just do git am mbox_file.mbox
> I don't know what a test/plain file means.
>
> On 12/07/2017 01:57 PM, Garming Sam via samba-technical wrote:
>> Hi,
>>
>> I'm fine with the changes in gpclass.py. I've also checked that all the
>> tests pass.
>>
>> In find_attr_val, you declared a variable in the for loop, which we
>> don't allow right now. But as metze has said, you should just replace
>> this function with the existing helper methods in ldb.
>>
>>
>> Cheers,
>>
>> Garming
>>
>> On 07/12/17 19:57, Stefan Metzmacher via samba-technical wrote:
>>> Hi David,
>>>
>>> would it be possible that you attach patches
>>> as test/plain instead of application/mbox?
>>> That would make it much easier to review the patches (at least for me)
>>>
>>> Can you please make use of existing helper functions in
>>> source4/torture/gpo/apply.c instead of introducing find_attr_val()
>>> I guess ldb_msg_find_attr_as_string() can be used instead.
>>>
>>> Using val = ldb_msg_find_ldb_val() and then
>>> (char*)val->data should be also be replaced by
>>> ldb_msg_find_attr_as_string().
>>>
>>> ldb_msg_find_attr_as_[u]int*() together with torture_assert_int_equal()
>>> should be used instead of using atoi() and torture_assert().
>>>
>>> Please let someone else review the gpclass.py I don't have time to
>>> wrap my head around this currently.
>>>
>>> Thanks!
>>> metze
>>>
>>> Am 06.12.2017 um 19:23 schrieb David Mulder via samba-technical:
>>>> Fixes for 2 bugs in the gpo unapply.
>>>> * The apply log was being left empty. Apparently pointers to tree
>>>> elements can't be stored in an object, they somehow are disconnected
>>>> from the main tree and nothing gets logged. These have to be found each
>>>> time we want to add something. This is probably a bug in python ElementTree.
>>>> * We should only commit the earliest change from a gpo to the log.
>>>> Otherwise we overwrite the original value written by that gpo, and then
>>>> we tattoo the setting on unapply.
>>>> * Adds testing so this isn't missed again.
>>>>
>>>>   python/samba/gpclass.py               |  67 ++++++++++++++++++++----------------
>>>>   source4/scripting/bin/samba_gpoupdate |   4 +--
>>>>   source4/torture/gpo/apply.c           | 108 +++++++++++++++++++++++++++++++++++++++++++++++++----------
>>>>   3 files changed, 130 insertions(+), 49 deletions(-)
>>>>
>>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v2] Fix GPO unapply log

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi David,

> OK. I'll implement the suggestions here and post a follow up set of patches.
> metze... I typically use mbox because I can just do git am mbox_file.mbox
> I don't know what a test/plain file means.

It's about the content-type in the mail, maybe just name the file
mbox_file.mbox.txt and thunderbird will use text/plain.

metze

> On 12/07/2017 01:57 PM, Garming Sam via samba-technical wrote:
>> Hi,
>>
>> I'm fine with the changes in gpclass.py. I've also checked that all the
>> tests pass.
>>
>> In find_attr_val, you declared a variable in the for loop, which we
>> don't allow right now. But as metze has said, you should just replace
>> this function with the existing helper methods in ldb.
>>
>>
>> Cheers,
>>
>> Garming
>>
>> On 07/12/17 19:57, Stefan Metzmacher via samba-technical wrote:
>>> Hi David,
>>>
>>> would it be possible that you attach patches
>>> as test/plain instead of application/mbox?
>>> That would make it much easier to review the patches (at least for me)
>>>
>>> Can you please make use of existing helper functions in
>>> source4/torture/gpo/apply.c instead of introducing find_attr_val()
>>> I guess ldb_msg_find_attr_as_string() can be used instead.
>>>
>>> Using val = ldb_msg_find_ldb_val() and then
>>> (char*)val->data should be also be replaced by
>>> ldb_msg_find_attr_as_string().
>>>
>>> ldb_msg_find_attr_as_[u]int*() together with torture_assert_int_equal()
>>> should be used instead of using atoi() and torture_assert().
>>>
>>> Please let someone else review the gpclass.py I don't have time to
>>> wrap my head around this currently.
>>>
>>> Thanks!
>>> metze
>>>
>>> Am 06.12.2017 um 19:23 schrieb David Mulder via samba-technical:
>>>> Fixes for 2 bugs in the gpo unapply.
>>>> * The apply log was being left empty. Apparently pointers to tree
>>>> elements can't be stored in an object, they somehow are disconnected
>>>> from the main tree and nothing gets logged. These have to be found each
>>>> time we want to add something. This is probably a bug in python ElementTree.
>>>> * We should only commit the earliest change from a gpo to the log.
>>>> Otherwise we overwrite the original value written by that gpo, and then
>>>> we tattoo the setting on unapply.
>>>> * Adds testing so this isn't missed again.
>>>>
>>>>  python/samba/gpclass.py               |  67 ++++++++++++++++++++----------------
>>>>  source4/scripting/bin/samba_gpoupdate |   4 +--
>>>>  source4/torture/gpo/apply.c           | 108 +++++++++++++++++++++++++++++++++++++++++++++++++----------
>>>>  3 files changed, 130 insertions(+), 49 deletions(-)
>>>>
>>
>>
>


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES v2] Fix GPO unapply log

Samba - samba-technical mailing list
Ah, I see. I will do that.

On 12/08/2017 08:08 AM, Stefan Metzmacher wrote:

> Hi David,
>
>> OK. I'll implement the suggestions here and post a follow up set of patches.
>> metze... I typically use mbox because I can just do git am mbox_file.mbox
>> I don't know what a test/plain file means.
> It's about the content-type in the mail, maybe just name the file
> mbox_file.mbox.txt and thunderbird will use text/plain.
>
> metze
>
>> On 12/07/2017 01:57 PM, Garming Sam via samba-technical wrote:
>>> Hi,
>>>
>>> I'm fine with the changes in gpclass.py. I've also checked that all the
>>> tests pass.
>>>
>>> In find_attr_val, you declared a variable in the for loop, which we
>>> don't allow right now. But as metze has said, you should just replace
>>> this function with the existing helper methods in ldb.
>>>
>>>
>>> Cheers,
>>>
>>> Garming
>>>
>>> On 07/12/17 19:57, Stefan Metzmacher via samba-technical wrote:
>>>> Hi David,
>>>>
>>>> would it be possible that you attach patches
>>>> as test/plain instead of application/mbox?
>>>> That would make it much easier to review the patches (at least for me)
>>>>
>>>> Can you please make use of existing helper functions in
>>>> source4/torture/gpo/apply.c instead of introducing find_attr_val()
>>>> I guess ldb_msg_find_attr_as_string() can be used instead.
>>>>
>>>> Using val = ldb_msg_find_ldb_val() and then
>>>> (char*)val->data should be also be replaced by
>>>> ldb_msg_find_attr_as_string().
>>>>
>>>> ldb_msg_find_attr_as_[u]int*() together with torture_assert_int_equal()
>>>> should be used instead of using atoi() and torture_assert().
>>>>
>>>> Please let someone else review the gpclass.py I don't have time to
>>>> wrap my head around this currently.
>>>>
>>>> Thanks!
>>>> metze
>>>>
>>>> Am 06.12.2017 um 19:23 schrieb David Mulder via samba-technical:
>>>>> Fixes for 2 bugs in the gpo unapply.
>>>>> * The apply log was being left empty. Apparently pointers to tree
>>>>> elements can't be stored in an object, they somehow are disconnected
>>>>> from the main tree and nothing gets logged. These have to be found each
>>>>> time we want to add something. This is probably a bug in python ElementTree.
>>>>> * We should only commit the earliest change from a gpo to the log.
>>>>> Otherwise we overwrite the original value written by that gpo, and then
>>>>> we tattoo the setting on unapply.
>>>>> * Adds testing so this isn't missed again.
>>>>>
>>>>>  python/samba/gpclass.py               |  67 ++++++++++++++++++++----------------
>>>>>  source4/scripting/bin/samba_gpoupdate |   4 +--
>>>>>  source4/torture/gpo/apply.c           | 108 +++++++++++++++++++++++++++++++++++++++++++++++++----------
>>>>>  3 files changed, 130 insertions(+), 49 deletions(-)
>>>>>
>>>
>

--
David Mulder
SUSE Labs Software Engineer - Samba
[hidden email]
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)