Please see the attached patches. I tried to split the patches logically
into manageable sets.
Unfortunately I made a minor mistake and I am afraid I will do something
wrong to fix it.
I merged two wrong patches. Fortunately it was three liner with 1 liner
so it is not a big of the deal but I am really scared that I will do
something wrong and loose the work I have done.
So I hope it is Ok to send it as is.
0001--INI-Making-Coverity-happy.patch <- this is the patch I submitted
earlier that I merged by mistake. I was supposed to merge it with patch
25 but picked the wrong one instead.
Patch 25 addresses the real issue found by Coverity as mentioned in
Stephen's review mail but it did not apply cleanly since it relies on
some code from the patches in the middle.
0002--INI-Adding-missing-function-declararion.patch <- this is the
patch that was rejected from the second set sent earlier. Fixed
according to review comment.
0003--BUILD-Allow-trace-per-component.patch <- This patch allows tracing
per component
The following set of patches introduces the merging of sections during
the reading of the file:
0004--INI-New-error-codes-and-messages.patch
0005--INI-New-merge-flags.patch
0006--INI-Add-new-vars-to-parse-structure.patch
0007--INI-Add-save_error-function.patch
0008--INI-Change-parse_error-to-use-save_error.patch
0009--INI-Preparing-for-merging-sections.patch
0010--INI-Enhance-value-processing.patch
0011--INI-Use-section-line-number.patch
0012--INI-Refactor-section-processing.patch
0013--INI-Return-error-in-DETECT-mode.patch
0014--INI-New-test-files-for-section-merge.patch
0015--INI-Test-DETECT-mode-and-use-new-file.patch
0016--INI-Test-for-all-section-merge-modes.patch
Patches related porting of the meta data from old way of doing things to
the new way of doing things:
0017--INI-Separate-close-and-destroy.patch
0018--INI-Function-to-reopen-file.patch
0019--INI-Metadata-collection-is-gone.patch
0020--INI-Check-access-function.patch
0021--INI-Avoid-double-free.patch <- patch related to 17 (missed check)
0022--INI-Function-to-check-for-changes.patch
0023--INI-Tests-for-access-and-changes.patch
0024--INI-Rename-error-print-function.patch <- rename error printing
function for consistency with new interface
0025--INI-Initialize-variables-in-loops.patch <- Coverity issue
addressed. Related to patch 0001.
0026--INI-Exposing-functions.patch <- Make some internal functions reusable
There is also patch 27. It is a piece of new functionality. It is a
preview. Please see the comment before reviewing it.
Do I need to split it into multiple patches or it is Ok as is? It is
pretty big but all changes are in one file and logically related.
The UNIT test is missing so I am not claiming it actually works as
expected.
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

Please see the attached patches. I tried to split the patches
logically
into manageable sets.
Unfortunately I made a minor mistake and I am afraid I will do something
wrong to fix it.
I merged two wrong patches. Fortunately it was three liner with 1 liner
so it is not a big of the deal but I am really scared that I will do
something wrong and loose the work I have done.
So I hope it is Ok to send it as is.
0001--INI-Making-Coverity-happy.patch <- this is the patch I submitted
earlier that I merged by mistake. I was supposed to merge it with patch
25 but picked the wrong one instead.
Patch 25 addresses the real issue found by Coverity as mentioned in
Stephen's review mail but it did not apply cleanly since it relies on
some code from the patches in the middle.

I split this patch. I'm attaching the missing initializer patch. That
gets an ack.
However, the 100078/79 fix is not correct. I did some digging and found
the real problem. If you look at line 264 and 279 of ini_valueobj_ut.c,
it's possible for other_create_test() to return EOK without having
modified the vo variable. As a result, you will be passing the
uninitialized value to modify_test(). So Coverity is right that there's
a possibility of it passing a freed variable here.
So the Coverity fix gets a nack.

The following set of patches introduces the merging of sections
during
the reading of the file:
0004--INI-New-error-codes-and-messages.patch

It would be better to use an enum here instead of #defines, then your
last entry for ERR_MAXPARSE will always be one higher than your previous
error message.
See sdap_basic_opt in src/providers/ldap/sdap.h in the SSSD source code
for an example.
That said, there's nothing WRONG with this patch, so ack.

Nack. It doesn't
make sense to pass in an index value to an array local
only to the function. That's not clean. It would be better to either
pass in the const char * for the message, or at worst pass in an enum
type that you would use to look up the matching error message.

Patches related porting of the meta data from old way of doing things to
the new way of doing things:
0017--INI-Separate-close-and-destroy.patch
0018--INI-Function-to-reopen-file.patch
0019--INI-Metadata-collection-is-gone.patch
0020--INI-Check-access-function.patch
0021--INI-Avoid-double-free.patch <- patch related to 17 (missed check)
0022--INI-Function-to-check-for-changes.patch
0023--INI-Tests-for-access-and-changes.patch
0024--INI-Rename-error-print-function.patch <- rename error printing
function for consistency with new interface
0025--INI-Initialize-variables-in-loops.patch <- Coverity issue
addressed. Related to patch 0001.
0026--INI-Exposing-functions.patch <- Make some internal functions reusable
There is also patch 27. It is a piece of new functionality. It is a
preview. Please see the comment before reviewing it.
Do I need to split it into multiple patches or it is Ok as is? It is
pretty big but all changes are in one file and logically related.
The UNIT test is missing so I am not claiming it actually works as
expected.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel

On 01/03/2011 06:12 PM, Dmitri Pal wrote:
> Please see the attached patches. I tried to split the patches logically
> into manageable sets.
> Unfortunately I made a minor mistake and I am afraid I will do something
> wrong to fix it.
> I merged two wrong patches. Fortunately it was three liner with 1 liner
> so it is not a big of the deal but I am really scared that I will do
> something wrong and loose the work I have done.
> So I hope it is Ok to send it as is.
> 0001--INI-Making-Coverity-happy.patch <- this is the patch I submitted
> earlier that I merged by mistake. I was supposed to merge it with patch
> 25 but picked the wrong one instead.
> Patch 25 addresses the real issue found by Coverity as mentioned in
> Stephen's review mail but it did not apply cleanly since it relies on
> some code from the patches in the middle.
I split this patch. I'm attaching the missing initializer patch. That
gets an ack.
However, the 100078/79 fix is not correct. I did some digging and found
the real problem. If you look at line 264 and 279 of ini_valueobj_ut.c,
it's possible for other_create_test() to return EOK without having
modified the vo variable. As a result, you will be passing the
uninitialized value to modify_test(). So Coverity is right that there's
a possibility of it passing a freed variable here.
So the Coverity fix gets a nack.

As I understand the changes I made are still valid. They are just not
enough. So the fix will be incremental. Right?

This is exactly what I did not like to do and tried to avoid. It is more
code on the configure script, more checks and less convenient to turn on
and off becuase you can run the same configure command with one switch
enabled from history and do "touch trace" & "rm trace" as needed
in the
directory you need.
Saves a lot of typing for me :-)
If you do not like the patch I will have to keep it as private patch as
it is more convenient for me during development than the flags.

> The following set of patches introduces the merging of sections during
> the reading of the file:
> 0004--INI-New-error-codes-and-messages.patch
It would be better to use an enum here instead of #defines, then your
last entry for ERR_MAXPARSE will always be one higher than your previous
error message.
See sdap_basic_opt in src/providers/ldap/sdap.h in the SSSD source code
for an example.
That said, there's nothing WRONG with this patch, so ack.

> 0005--INI-New-merge-flags.patch
I don't much like the idea of having flags that have overlapping bits
without an obvious reason (0x0020 and 0x0030, for example), but since
those are pre-existing, I'll leave them alone. Ack.

Can you please elaborate becuase I designed the whole set of flags to be
overlapping on purpose.
There are three ranges that address three use cases:
1) Flags that define how to merge values that are encountered within the
same section
[section]
a=1
a=2
2) Flags that define how to merge values that are in a section that is
segmented
[section]
a=1
...
[section]
a=2
3) Flags that define how to merge sections
But all three are related (especially case 2 & 3) so it makes sense to
pass them in one variable
But if there are some concerns or I am missing something please explain.

> 0006--INI-Add-new-vars-to-parse-structure.patch
Ack, though it seems to me that a memset to zero would be simpler than
manually setting every struct member to zero manually.

I dislike calloc. May be I am blowing on water. It is just matter of style.
Tried to change this attitude and use calloc or memset - does not fit
well with me.
It is a little bit more code but sometimes more initialization is needed
and habit of initializing each individual member pays off. This is the
case when actually I missed it. Thanks to Coverity that found the issue.

> 0007--INI-Add-save_error-function.patch
Nack. It doesn't make sense to pass in an index value to an array local
only to the function. That's not clean. It would be better to either
pass in the const char * for the message, or at worst pass in an enum
type that you would use to look up the matching error message.

Hm. Ok. I see how it violates the "defensive programming" paradigm.
Old habits hard to get rid of. ;-)

> 0008--INI-Change-parse_error-to-use-save_error.patch
Nack. This will need to be updated to correspond to the changes for
patch 0007.
> 0009--INI-Preparing-for-merging-sections.patch
Ack.
> 0010--INI-Enhance-value-processing.patch
Ack.
> 0011--INI-Use-section-line-number.patch
Ack.
> 0012--INI-Refactor-section-processing.patch
Nack. Please fix the formatting of the switch statement. There should be
only one level of indent following the case tag.
> 0013--INI-Return-error-in-DETECT-mode.patch
Ack
> 0014--INI-New-test-files-for-section-merge.patch
Ack
> 0015--INI-Test-DETECT-mode-and-use-new-file.patch
Ack
> 0016--INI-Test-for-all-section-merge-modes.patch
Nack. Please fix random tabs in the indentation. Otherwise it looks fine.
I'll review the porting patches in a separate email. This should be
enough to get you started in the meantime.
> Patches related porting of the meta data from old way of doing things to
> the new way of doing things:
> 0017--INI-Separate-close-and-destroy.patch
> 0018--INI-Function-to-reopen-file.patch
> 0019--INI-Metadata-collection-is-gone.patch
> 0020--INI-Check-access-function.patch
> 0021--INI-Avoid-double-free.patch <- patch related to 17 (missed check)
> 0022--INI-Function-to-check-for-changes.patch
> 0023--INI-Tests-for-access-and-changes.patch
> 0024--INI-Rename-error-print-function.patch <- rename error printing
> function for consistency with new interface
> 0025--INI-Initialize-variables-in-loops.patch <- Coverity issue
> addressed. Related to patch 0001.
> 0026--INI-Exposing-functions.patch <- Make some internal functions
reusable
> There is also patch 27. It is a piece of new functionality. It is a
> preview. Please see the comment before reviewing it.
> Do I need to split it into multiple patches or it is Ok as is? It is
> pretty big but all changes are in one file and logically related.
> The UNIT test is missing so I am not claiming it actually works as
> expected.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel

Thank you for the review.
Questions/comments inline.
Stephen Gallagher wrote:
> On 01/03/2011 06:12 PM, Dmitri Pal wrote:
>> Please see the attached patches. I tried to split the patches logically
>> into manageable sets.
>> Unfortunately I made a minor mistake and I am afraid I will do something
>> wrong to fix it.
>> I merged two wrong patches. Fortunately it was three liner with 1 liner
>> so it is not a big of the deal but I am really scared that I will do
>> something wrong and loose the work I have done.
>> So I hope it is Ok to send it as is.
>
>> 0001--INI-Making-Coverity-happy.patch <- this is the patch I submitted
>> earlier that I merged by mistake. I was supposed to merge it with patch
>> 25 but picked the wrong one instead.
>> Patch 25 addresses the real issue found by Coverity as mentioned in
>> Stephen's review mail but it did not apply cleanly since it relies on
>> some code from the patches in the middle.
>
>
> I split this patch. I'm attaching the missing initializer patch. That
> gets an ack.
>
> However, the 100078/79 fix is not correct. I did some digging and found
> the real problem. If you look at line 264 and 279 of ini_valueobj_ut.c,
> it's possible for other_create_test() to return EOK without having
> modified the vo variable. As a result, you will be passing the
> uninitialized value to modify_test(). So Coverity is right that there's
> a possibility of it passing a freed variable here.
>
> So the Coverity fix gets a nack.
As I understand the changes I made are still valid. They are just not
enough. So the fix will be incremental. Right?

>
>
>> 0003--BUILD-Allow-trace-per-component.patch <- This patch allows tracing
>> per component
>
>
> Nack. Requiring a file doesn't make sense. Just add --enable-trace-ini,
> --enable-trace-collection etc. flags.
This is exactly what I did not like to do and tried to avoid. It is more
code on the configure script, more checks and less convenient to turn on
and off becuase you can run the same configure command with one switch
enabled from history and do "touch trace" & "rm trace" as needed
in the
directory you need.
Saves a lot of typing for me :-)
If you do not like the patch I will have to keep it as private patch as
it is more convenient for me during development than the flags.

>
>> The following set of patches introduces the merging of sections during
>> the reading of the file:
>> 0004--INI-New-error-codes-and-messages.patch
>
> It would be better to use an enum here instead of #defines, then your
> last entry for ERR_MAXPARSE will always be one higher than your previous
> error message.
>
> See sdap_basic_opt in src/providers/ldap/sdap.h in the SSSD source code
> for an example.
>
> That said, there's nothing WRONG with this patch, so ack.
Yes I know that enum would be stylistically better. May be you are right
and I should switch to ENUM. I will open a ticket for that.
https://fedorahosted.org/sssd/ticket/766

>
>> 0005--INI-New-merge-flags.patch
> I don't much like the idea of having flags that have overlapping bits
> without an obvious reason (0x0020 and 0x0030, for example), but since
> those are pre-existing, I'll leave them alone. Ack.
Can you please elaborate becuase I designed the whole set of flags to be
overlapping on purpose.
There are three ranges that address three use cases:
1) Flags that define how to merge values that are encountered within the
same section
[section]
a=1
a=2
2) Flags that define how to merge values that are in a section that is
segmented
[section]
a=1
...
[section]
a=2
3) Flags that define how to merge sections
But all three are related (especially case 2 & 3) so it makes sense to
pass them in one variable
But if there are some concerns or I am missing something please explain.

#define INI_MV1S_OVERWRITE 0x0000
#define INI_MV1S_ERROR 0x0001
#define INI_MV1S_PRESERVE 0x0002
#define INI_MV1S_ALLOW 0x0003
#define INI_MV1S_DETECT 0x0004
I don't like that on a bitwise compare, if you do:
var = INI_MV1S_ALLOW;
then
if (var & INI_MV1S_ERROR) {
}
would be true. This isn't how flags are supposed to work. You're using
these more like an enum. It would be better to define them as such (and
not call them "flags" if that's what you're trying to do.
Calling them flags implies that you would expect to be able to do:
var = INI_MV1S_OVERWRITE | INI_MV1S_OVERWRITE;
And have both of those be active.

>
>> 0006--INI-Add-new-vars-to-parse-structure.patch
> Ack, though it seems to me that a memset to zero would be simpler than
> manually setting every struct member to zero manually.
I dislike calloc. May be I am blowing on water. It is just matter of style.
Tried to change this attitude and use calloc or memset - does not fit
well with me.
It is a little bit more code but sometimes more initialization is needed
and habit of initializing each individual member pays off. This is the
case when actually I missed it. Thanks to Coverity that found the issue.

Right, but memsetting to zero guarantees that everything is intialized.
Like I said, the patch is acked, but it's not my favorite style :)
- --
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/

>>
>>> 0003--BUILD-Allow-trace-per-component.patch <- This patch allows
tracing
>>> per component
>>
>> Nack. Requiring a file doesn't make sense. Just add --enable-trace-ini,
>> --enable-trace-collection etc. flags.
> This is exactly what I did not like to do and tried to avoid. It is more
> code on the configure script, more checks and less convenient to turn on
> and off becuase you can run the same configure command with one switch
> enabled from history and do "touch trace" & "rm trace" as
needed in the
> directory you need.
> Saves a lot of typing for me :-)
> If you do not like the patch I will have to keep it as private patch as
> it is more convenient for me during development than the flags.
I still vote nack, as I don't like the approach.

So this is the dead lock becuase I like the approach and do not like
yours. How we are going to resolve it?

>>> 0005--INI-New-merge-flags.patch
>> I don't much like the idea of having flags that have overlapping bits
>> without an obvious reason (0x0020 and 0x0030, for example), but since
>> those are pre-existing, I'll leave them alone. Ack.
> Can you please elaborate becuase I designed the whole set of flags to be
> overlapping on purpose.
> There are three ranges that address three use cases:
> 1) Flags that define how to merge values that are encountered within the
> same section
> [section]
> a=1
> a=2
> 2) Flags that define how to merge values that are in a section that is
> segmented
> [section]
> a=1
> ...
> [section]
> a=2
> 3) Flags that define how to merge sections
> But all three are related (especially case 2 & 3) so it makes sense to
> pass them in one variable
> But if there are some concerns or I am missing something please explain.
#define INI_MV1S_OVERWRITE 0x0000
#define INI_MV1S_ERROR 0x0001
#define INI_MV1S_PRESERVE 0x0002
#define INI_MV1S_ALLOW 0x0003
#define INI_MV1S_DETECT 0x0004
I don't like that on a bitwise compare, if you do:
var = INI_MV1S_ALLOW;
then
if (var & INI_MV1S_ERROR) {
}
would be true. This isn't how flags are supposed to work. You're using
these more like an enum. It would be better to define them as such (and
not call them "flags" if that's what you're trying to do.
Calling them flags implies that you would expect to be able to do:
var = INI_MV1S_OVERWRITE | INI_MV1S_OVERWRITE;
And have both of those be active.

I view it differently. Within the range they are mutually exclusive but
they are flags since I expect it to be used in a following way:
var = INI_MS_MERGE | INI_MV1S_OVERWRITE | INI_MV2S_OVERWRITE;
Do you have a better way of accomplishing what I am trying to do?
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

>
>>>
>>>> 0003--BUILD-Allow-trace-per-component.patch <- This patch allows
> tracing
>>>> per component
>>>
>>> Nack. Requiring a file doesn't make sense. Just add --enable-trace-ini,
>>> --enable-trace-collection etc. flags.
>> This is exactly what I did not like to do and tried to avoid. It is more
>> code on the configure script, more checks and less convenient to turn on
>> and off becuase you can run the same configure command with one switch
>> enabled from history and do "touch trace" & "rm trace" as
needed in the
>> directory you need.
>> Saves a lot of typing for me :-)
>> If you do not like the patch I will have to keep it as private patch as
>> it is more convenient for me during development than the flags.
>
>
> I still vote nack, as I don't like the approach.
So this is the dead lock becuase I like the approach and do not like
yours. How we are going to resolve it?

Ultimately, it's your project, not mine. You have commit privilege and
full discretion on when to use it. You asked for my review comments. You
don't have to listen to them :)

>
>
>>>> 0005--INI-New-merge-flags.patch
>>> I don't much like the idea of having flags that have overlapping bits
>>> without an obvious reason (0x0020 and 0x0030, for example), but since
>>> those are pre-existing, I'll leave them alone. Ack.
>> Can you please elaborate becuase I designed the whole set of flags to be
>> overlapping on purpose.
>> There are three ranges that address three use cases:
>> 1) Flags that define how to merge values that are encountered within the
>> same section
>> [section]
>> a=1
>> a=2
>> 2) Flags that define how to merge values that are in a section that is
>> segmented
>> [section]
>> a=1
>> ...
>> [section]
>> a=2
>> 3) Flags that define how to merge sections
>> But all three are related (especially case 2 & 3) so it makes sense to
>> pass them in one variable
>
>> But if there are some concerns or I am missing something please explain.
>
>
>
> #define INI_MV1S_OVERWRITE 0x0000
> #define INI_MV1S_ERROR 0x0001
> #define INI_MV1S_PRESERVE 0x0002
> #define INI_MV1S_ALLOW 0x0003
> #define INI_MV1S_DETECT 0x0004
>
> I don't like that on a bitwise compare, if you do:
> var = INI_MV1S_ALLOW;
>
> then
>
> if (var & INI_MV1S_ERROR) {
> }
> would be true. This isn't how flags are supposed to work. You're using
> these more like an enum. It would be better to define them as such (and
> not call them "flags" if that's what you're trying to do.
>
> Calling them flags implies that you would expect to be able to do:
>
> var = INI_MV1S_OVERWRITE | INI_MV1S_OVERWRITE;
>
> And have both of those be active.
I view it differently. Within the range they are mutually exclusive but
they are flags since I expect it to be used in a following way:
var = INI_MS_MERGE | INI_MV1S_OVERWRITE | INI_MV2S_OVERWRITE;
Do you have a better way of accomplishing what I am trying to do?

On 01/05/2011 03:40 PM, Dmitri Pal wrote:
>>>>> 0003--BUILD-Allow-trace-per-component.patch <- This patch allows
>> tracing
>>>>> per component
>>>> Nack. Requiring a file doesn't make sense. Just add
--enable-trace-ini,
>>>> --enable-trace-collection etc. flags.
>>> This is exactly what I did not like to do and tried to avoid. It
is more
>>> code on the configure script, more checks and less convenient to
turn on
>>> and off becuase you can run the same configure command with one switch
>>> enabled from history and do "touch trace" & "rm
trace" as needed
in the
>>> directory you need.
>>> Saves a lot of typing for me :-)
>>> If you do not like the patch I will have to keep it as private
patch as
>>> it is more convenient for me during development than the flags.
>>
>> I still vote nack, as I don't like the approach.
> So this is the dead lock becuase I like the approach and do not like
> yours. How we are going to resolve it?
Ultimately, it's your project, not mine. You have commit privilege and
full discretion on when to use it. You asked for my review comments. You
don't have to listen to them :)

Nack. I think we need to come to some terms and find consensus. I will
definitely not push it over your head using my privilege.
I agree that what you suggest is logical and consistent with how things
are done. The problem is that it does not help me much.
I will keep patch in my tree for now.

>>
>>>>> 0005--INI-New-merge-flags.patch
>>>> I don't much like the idea of having flags that have overlapping
bits
>>>> without an obvious reason (0x0020 and 0x0030, for example), but since
>>>> those are pre-existing, I'll leave them alone. Ack.
>>> Can you please elaborate becuase I designed the whole set of flags
to be
>>> overlapping on purpose.
>>> There are three ranges that address three use cases:
>>> 1) Flags that define how to merge values that are encountered
within the
>>> same section
>>> [section]
>>> a=1
>>> a=2
>>> 2) Flags that define how to merge values that are in a section that is
>>> segmented
>>> [section]
>>> a=1
>>> ...
>>> [section]
>>> a=2
>>> 3) Flags that define how to merge sections
>>> But all three are related (especially case 2 & 3) so it makes sense to
>>> pass them in one variable
>>> But if there are some concerns or I am missing something please
explain.
>>
>>
>> #define INI_MV1S_OVERWRITE 0x0000
>> #define INI_MV1S_ERROR 0x0001
>> #define INI_MV1S_PRESERVE 0x0002
>> #define INI_MV1S_ALLOW 0x0003
>> #define INI_MV1S_DETECT 0x0004
>>
>> I don't like that on a bitwise compare, if you do:
>> var = INI_MV1S_ALLOW;
>>
>> then
>>
>> if (var & INI_MV1S_ERROR) {
>> }
>> would be true. This isn't how flags are supposed to work. You're using
>> these more like an enum. It would be better to define them as such (and
>> not call them "flags" if that's what you're trying to do.
>>
>> Calling them flags implies that you would expect to be able to do:
>>
>> var = INI_MV1S_OVERWRITE | INI_MV1S_OVERWRITE;
>>
>> And have both of those be active.
> I view it differently. Within the range they are mutually exclusive but
> they are flags since I expect it to be used in a following way:
> var = INI_MS_MERGE | INI_MV1S_OVERWRITE | INI_MV2S_OVERWRITE;
> Do you have a better way of accomplishing what I am trying to do?
I think this would be better served if var was:
struct ini_struct {
enum ms_cmd ms;
enum mv1s_cmd mv1s;
enum mv2s_cmd mv2s;
}
This would guarantee the mutual exclusivity.

Stephen Gallagher wrote:
> On 01/05/2011 03:40 PM, Dmitri Pal wrote:
> >>>>> 0003--BUILD-Allow-trace-per-component.patch <- This patch
allows
> >> tracing
> >>>>> per component
> >>>> Nack. Requiring a file doesn't make sense. Just add
> --enable-trace-ini,
> >>>> --enable-trace-collection etc. flags.
> >>> This is exactly what I did not like to do and tried to avoid. It
> is more
> >>> code on the configure script, more checks and less convenient to
> turn on
> >>> and off becuase you can run the same configure command with one switch
> >>> enabled from history and do "touch trace" & "rm
trace" as needed
> in the
> >>> directory you need.
> >>> Saves a lot of typing for me :-)
> >>> If you do not like the patch I will have to keep it as private
> patch as
> >>> it is more convenient for me during development than the flags.
> >>
> >> I still vote nack, as I don't like the approach.
> > So this is the dead lock becuase I like the approach and do not like
> > yours. How we are going to resolve it?
>
>
> Ultimately, it's your project, not mine. You have commit privilege and
> full discretion on when to use it. You asked for my review comments. You
> don't have to listen to them :)
Nack. I think we need to come to some terms and find consensus. I will
definitely not push it over your head using my privilege.
I agree that what you suggest is logical and consistent with how things
are done. The problem is that it does not help me much.
I will keep patch in my tree for now.

To enlarge the quorum I throw in my opinion here. In general I agree
with Stephen because the file approach is quite unusual and needs to be
well documented so that other developers can use it. You might also want
to use $builddir instead of $srcdir to allow a separate build directory.
But I would like to suggest another approach which might be even easier
to use. You can introduce a make target like 'trace' or 'devel' which
will build everything with tracing enabled. This way you do not even
have to rerun configure to enable tracing.
bye,
Sumit

On Wed, Jan 05, 2011 at 04:03:38PM -0500, Dmitri Pal wrote:
> Stephen Gallagher wrote:
>> On 01/05/2011 03:40 PM, Dmitri Pal wrote:
>>>>>>> 0003--BUILD-Allow-trace-per-component.patch <- This patch
allows
>>>> tracing
>>>>>>> per component
>>>>>> Nack. Requiring a file doesn't make sense. Just add
>> --enable-trace-ini,
>>>>>> --enable-trace-collection etc. flags.
>>>>> This is exactly what I did not like to do and tried to avoid. It
>> is more
>>>>> code on the configure script, more checks and less convenient to
>> turn on
>>>>> and off becuase you can run the same configure command with one
switch
>>>>> enabled from history and do "touch trace" & "rm
trace" as needed
>> in the
>>>>> directory you need.
>>>>> Saves a lot of typing for me :-)
>>>>> If you do not like the patch I will have to keep it as private
>> patch as
>>>>> it is more convenient for me during development than the flags.
>>>>
>>>> I still vote nack, as I don't like the approach.
>>> So this is the dead lock becuase I like the approach and do not like
>>> yours. How we are going to resolve it?
>>
>>
>> Ultimately, it's your project, not mine. You have commit privilege and
>> full discretion on when to use it. You asked for my review comments. You
>> don't have to listen to them :)
>
> Nack. I think we need to come to some terms and find consensus. I will
> definitely not push it over your head using my privilege.
> I agree that what you suggest is logical and consistent with how things
> are done. The problem is that it does not help me much.
> I will keep patch in my tree for now.
To enlarge the quorum I throw in my opinion here. In general I agree
with Stephen because the file approach is quite unusual and needs to be
well documented so that other developers can use it. You might also want
to use $builddir instead of $srcdir to allow a separate build directory.
But I would like to suggest another approach which might be even easier
to use. You can introduce a make target like 'trace' or 'devel' which
will build everything with tracing enabled. This way you do not even
have to rerun configure to enable tracing.

That's a good idea. The trace level could then be set by environment
variable as well.
e.g.
./configure
COL_TRACE=3 make devel
And this would tell it to pass -DTRACE_LEVEL=3 to the COLLECTION_CFLAGS
- --
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/

On 01/06/2011 03:02 AM, Sumit Bose wrote:
> On Wed, Jan 05, 2011 at 04:03:38PM -0500, Dmitri Pal wrote:
>> Stephen Gallagher wrote:
>>> On 01/05/2011 03:40 PM, Dmitri Pal wrote:
>>>>>>>> 0003--BUILD-Allow-trace-per-component.patch <- This
patch allows
>>>>> tracing
>>>>>>>> per component
>>>>>>> Nack. Requiring a file doesn't make sense. Just add
>>> --enable-trace-ini,
>>>>>>> --enable-trace-collection etc. flags.
>>>>>> This is exactly what I did not like to do and tried to avoid.
It
>>> is more
>>>>>> code on the configure script, more checks and less convenient
to
>>> turn on
>>>>>> and off becuase you can run the same configure command with one
switch
>>>>>> enabled from history and do "touch trace" &
"rm trace" as needed
>>> in the
>>>>>> directory you need.
>>>>>> Saves a lot of typing for me :-)
>>>>>> If you do not like the patch I will have to keep it as private
>>> patch as
>>>>>> it is more convenient for me during development than the flags.
>>>>> I still vote nack, as I don't like the approach.
>>>> So this is the dead lock becuase I like the approach and do not like
>>>> yours. How we are going to resolve it?
>>>
>>> Ultimately, it's your project, not mine. You have commit privilege and
>>> full discretion on when to use it. You asked for my review
comments. You
>>> don't have to listen to them :)
>> Nack. I think we need to come to some terms and find consensus. I will
>> definitely not push it over your head using my privilege.
>> I agree that what you suggest is logical and consistent with how things
>> are done. The problem is that it does not help me much.
>> I will keep patch in my tree for now.
> To enlarge the quorum I throw in my opinion here. In general I agree
> with Stephen because the file approach is quite unusual and needs to be
> well documented so that other developers can use it. You might also want
> to use $builddir instead of $srcdir to allow a separate build directory.
> But I would like to suggest another approach which might be even easier
> to use. You can introduce a make target like 'trace' or 'devel'
which
> will build everything with tracing enabled. This way you do not even
> have to rerun configure to enable tracing.
That's a good idea. The trace level could then be set by environment
variable as well.
e.g.
./configure
COL_TRACE=3 make devel
And this would tell it to pass -DTRACE_LEVEL=3 to the COLLECTION_CFLAGS

> 0001--INI-Making-Coverity-happy.patch <- this is the patch I submitted
> earlier that I merged by mistake. I was supposed to merge it with patch
> 25 but picked the wrong one instead.
> Patch 25 addresses the real issue found by Coverity as mentioned in
> Stephen's review mail but it did not apply cleanly since it relies on
> some code from the patches in the middle.
I split this patch. I'm attaching the missing initializer patch. That
gets an ack.

I am missing something. If you split the patch should there have been
two patches?
You sent just one.
Thanks
Dmitri

>
>> 0001--INI-Making-Coverity-happy.patch <- this is the patch I submitted
>> earlier that I merged by mistake. I was supposed to merge it with patch
>> 25 but picked the wrong one instead.
>> Patch 25 addresses the real issue found by Coverity as mentioned in
>> Stephen's review mail but it did not apply cleanly since it relies on
>> some code from the patches in the middle.
>
>
> I split this patch. I'm attaching the missing initializer patch. That
> gets an ack.
I am missing something. If you split the patch should there have been
two patches?
You sent just one.

I didn't send the other one because I had nacked it.
- --
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/

On 01/03/2011 06:12 PM, Dmitri Pal wrote:
> Please see the attached patches. I tried to split the patches logically
> into manageable sets.
> Unfortunately I made a minor mistake and I am afraid I will do something
> wrong to fix it.
> I merged two wrong patches. Fortunately it was three liner with 1 liner
> so it is not a big of the deal but I am really scared that I will do
> something wrong and loose the work I have done.
> So I hope it is Ok to send it as is.
> 0001--INI-Making-Coverity-happy.patch <- this is the patch I submitted
> earlier that I merged by mistake. I was supposed to merge it with patch
> 25 but picked the wrong one instead.
> Patch 25 addresses the real issue found by Coverity as mentioned in
> Stephen's review mail but it did not apply cleanly since it relies on
> some code from the patches in the middle.
I split this patch. I'm attaching the missing initializer patch. That
gets an ack.
However, the 100078/79 fix is not correct. I did some digging and found
the real problem. If you look at line 264 and 279 of ini_valueobj_ut.c,
it's possible for other_create_test() to return EOK without having
modified the vo variable. As a result, you will be passing the
uninitialized value to modify_test(). So Coverity is right that there's
a possibility of it passing a freed variable here.
So the Coverity fix gets a nack.

> The following set of patches introduces the merging of sections
during
> the reading of the file:
> 0004--INI-New-error-codes-and-messages.patch
It would be better to use an enum here instead of #defines, then your
last entry for ERR_MAXPARSE will always be one higher than your previous
error message.
See sdap_basic_opt in src/providers/ldap/sdap.h in the SSSD source code
for an example.
That said, there's nothing WRONG with this patch, so ack.

> 0005--INI-New-merge-flags.patch
I don't much like the idea of having flags that have overlapping bits
without an obvious reason (0x0020 and 0x0030, for example), but since
those are pre-existing, I'll leave them alone. Ack.

As I mentioned earlier I will reconsider however for now it is unchanged.

> 0007--INI-Add-save_error-function.patch
Nack. It doesn't make sense to pass in an index value to an array local
only to the function. That's not clean. It would be better to either
pass in the const char * for the message, or at worst pass in an enum
type that you would use to look up the matching error message.

Fixed.
Added another patch, in the new numeration it is patch 15. It is a
cosmetic change to meet the coding standard.
The rest are deferred in this round.
------------------------------
I also have a question.
I use a struct that is not aligned by 8 byte boundary on a 64 bit machine.
pointer = 8
unsigned = 4
unsigned = 4
unsigned = 4
total length is 20 which is padded to 24
I allocate memory for it using sizeof of the structure. Everything is
fine - no leak. I then set the values of the structure one by one.
Also in some cases for debugging purposes I print the contents of the
allocated memory as binary memory byte by byte.
When I do this with the memory used by this structure valgrind complains
that I am printing uninitialized memory. And indeed the padding is not
initialized by me.
So I can:
1) memset memory after allocating it (which I do not like to do
especially if I can set all the elements to the non zero values anyways
except for padding)
2) Add artificial padding to the structure or a dummy unsigned member
(seems a bit ugly)
3) Ignore the warning (seems wrong)
4) Ask your advice (...?)
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

On 01/05/2011 01:19 PM, Stephen Gallagher wrote:
> On 01/03/2011 06:12 PM, Dmitri Pal wrote:
> > Please see the attached patches. I tried to split the patches logically
> > into manageable sets.
> > Unfortunately I made a minor mistake and I am afraid I will do something
> > wrong to fix it.
> > I merged two wrong patches. Fortunately it was three liner with 1 liner
> > so it is not a big of the deal but I am really scared that I will do
> > something wrong and loose the work I have done.
> > So I hope it is Ok to send it as is.
>
> > 0001--INI-Making-Coverity-happy.patch <- this is the patch I submitted
> > earlier that I merged by mistake. I was supposed to merge it with patch
> > 25 but picked the wrong one instead.
> > Patch 25 addresses the real issue found by Coverity as mentioned in
> > Stephen's review mail but it did not apply cleanly since it relies on
> > some code from the patches in the middle.
>
>
> I split this patch. I'm attaching the missing initializer patch. That
> gets an ack.
>
> However, the 100078/79 fix is not correct. I did some digging and found
> the real problem. If you look at line 264 and 279 of ini_valueobj_ut.c,
> it's possible for other_create_test() to return EOK without having
> modified the vo variable. As a result, you will be passing the
> uninitialized value to modify_test(). So Coverity is right that there's
> a possibility of it passing a freed variable here.
>
> So the Coverity fix gets a nack.
>
I realized what the problem is. See the first patch.
>
> > 0002--INI-Adding-missing-function-declararion.patch <- this is the
> > patch that was rejected from the second set sent earlier. Fixed
> > according to review comment.
Dropped. I think Jakub fixed it.
>
>
> Ack.
>
>
> > 0003--BUILD-Allow-trace-per-component.patch <- This patch allows tracing
> > per component
>
>
> Nack. Requiring a file doesn't make sense. Just add --enable-trace-ini,
> --enable-trace-collection etc. flags.
>
Dropped for now
> > The following set of patches introduces the merging of sections during
> > the reading of the file:
> > 0004--INI-New-error-codes-and-messages.patch
>
> It would be better to use an enum here instead of #defines, then your
> last entry for ERR_MAXPARSE will always be one higher than your previous
> error message.
>
> See sdap_basic_opt in src/providers/ldap/sdap.h in the SSSD source code
> for an example.
>
> That said, there's nothing WRONG with this patch, so ack.
>
Attached as is. The ticket is open.
> > 0005--INI-New-merge-flags.patch
> I don't much like the idea of having flags that have overlapping bits
> without an obvious reason (0x0020 and 0x0030, for example), but since
> those are pre-existing, I'll leave them alone. Ack.
As I mentioned earlier I will reconsider however for now it is unchanged.
>
> > 0006--INI-Add-new-vars-to-parse-structure.patch
> Ack, though it seems to me that a memset to zero would be simpler than
> manually setting every struct member to zero manually.
Unchanged
>
> > 0007--INI-Add-save_error-function.patch
> Nack. It doesn't make sense to pass in an index value to an array local
> only to the function. That's not clean. It would be better to either
> pass in the const char * for the message, or at worst pass in an enum
> type that you would use to look up the matching error message.
Modified. Const char * is passed in.
>
> > 0008--INI-Change-parse_error-to-use-save_error.patch
> Nack. This will need to be updated to correspond to the changes for
> patch 0007.
Updated to reflect the above patch.
>
> > 0009--INI-Preparing-for-merging-sections.patch
> Ack.
>
Unchanged
> > 0010--INI-Enhance-value-processing.patch
> Ack.
>
Unchanged
> > 0011--INI-Use-section-line-number.patch
> Ack.
>
Unchanged
> > 0012--INI-Refactor-section-processing.patch
> Nack. Please fix the formatting of the switch statement. There should be
> only one level of indent following the case tag.
>
Done
> > 0013--INI-Return-error-in-DETECT-mode.patch
> Ack
>
Unchanged
> > 0014--INI-New-test-files-for-section-merge.patch
> Ack
>
Unchanged
> > 0015--INI-Test-DETECT-mode-and-use-new-file.patch
> Ack
>
Unchanged
> > 0016--INI-Test-for-all-section-merge-modes.patch
> Nack. Please fix random tabs in the indentation. Otherwise it looks fine.
>
Fixed.
Added another patch, in the new numeration it is patch 15. It is a
cosmetic change to meet the coding standard.
The rest are deferred in this round.
------------------------------
I also have a question.
I use a struct that is not aligned by 8 byte boundary on a 64 bit machine.
pointer = 8
unsigned = 4
unsigned = 4
unsigned = 4
total length is 20 which is padded to 24
I allocate memory for it using sizeof of the structure. Everything is
fine - no leak. I then set the values of the structure one by one.
Also in some cases for debugging purposes I print the contents of the
allocated memory as binary memory byte by byte.
When I do this with the memory used by this structure valgrind complains
that I am printing uninitialized memory. And indeed the padding is not
initialized by me.
So I can:
1) memset memory after allocating it (which I do not like to do
especially if I can set all the elements to the non zero values anyways
except for padding)
2) Add artificial padding to the structure or a dummy unsigned member
(seems a bit ugly)
3) Ignore the warning (seems wrong)
4) Ask your advice (...?)
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel

Thanks for review. I do not like memset(). It just hides things. If it
is not initialized coverity and valgrind would not complain and we will
see the issue.
The plugin for gedit that I use does not show the spaces the way the old
plugin did. I need to find a good plugin again.
I will take a note to find a better plugin.
Please push and I will just rebase.

On Fri, 2012-03-16 at 01:22 -0400, Dmitri Pal wrote:
> On 01/05/2011 01:19 PM, Stephen Gallagher wrote:
>> On 01/03/2011 06:12 PM, Dmitri Pal wrote:
>>> Please see the attached patches. I tried to split the patches logically
>>> into manageable sets.
>>> Unfortunately I made a minor mistake and I am afraid I will do something
>>> wrong to fix it.
>>> I merged two wrong patches. Fortunately it was three liner with 1 liner
>>> so it is not a big of the deal but I am really scared that I will do
>>> something wrong and loose the work I have done.
>>> So I hope it is Ok to send it as is.
>>> 0001--INI-Making-Coverity-happy.patch <- this is the patch I submitted
>>> earlier that I merged by mistake. I was supposed to merge it with patch
>>> 25 but picked the wrong one instead.
>>> Patch 25 addresses the real issue found by Coverity as mentioned in
>>> Stephen's review mail but it did not apply cleanly since it relies on
>>> some code from the patches in the middle.
>>
>> I split this patch. I'm attaching the missing initializer patch. That
>> gets an ack.
>>
>> However, the 100078/79 fix is not correct. I did some digging and found
>> the real problem. If you look at line 264 and 279 of ini_valueobj_ut.c,
>> it's possible for other_create_test() to return EOK without having
>> modified the vo variable. As a result, you will be passing the
>> uninitialized value to modify_test(). So Coverity is right that there's
>> a possibility of it passing a freed variable here.
>>
>> So the Coverity fix gets a nack.
>>
> I realized what the problem is. See the first patch.
>
>>> 0002--INI-Adding-missing-function-declararion.patch <- this is the
>>> patch that was rejected from the second set sent earlier. Fixed
>>> according to review comment.
> Dropped. I think Jakub fixed it.
>>
>> Ack.
>>
>>
>>> 0003--BUILD-Allow-trace-per-component.patch <- This patch allows tracing
>>> per component
>>
>> Nack. Requiring a file doesn't make sense. Just add --enable-trace-ini,
>> --enable-trace-collection etc. flags.
>>
> Dropped for now
>
>>> The following set of patches introduces the merging of sections during
>>> the reading of the file:
>>> 0004--INI-New-error-codes-and-messages.patch
>> It would be better to use an enum here instead of #defines, then your
>> last entry for ERR_MAXPARSE will always be one higher than your previous
>> error message.
>>
>> See sdap_basic_opt in src/providers/ldap/sdap.h in the SSSD source code
>> for an example.
>>
>> That said, there's nothing WRONG with this patch, so ack.
>>
> Attached as is. The ticket is open.
>
>
>>> 0005--INI-New-merge-flags.patch
>> I don't much like the idea of having flags that have overlapping bits
>> without an obvious reason (0x0020 and 0x0030, for example), but since
>> those are pre-existing, I'll leave them alone. Ack.
> As I mentioned earlier I will reconsider however for now it is unchanged.
>
>>> 0006--INI-Add-new-vars-to-parse-structure.patch
>> Ack, though it seems to me that a memset to zero would be simpler than
>> manually setting every struct member to zero manually.
> Unchanged
>
>
>>> 0007--INI-Add-save_error-function.patch
>> Nack. It doesn't make sense to pass in an index value to an array local
>> only to the function. That's not clean. It would be better to either
>> pass in the const char * for the message, or at worst pass in an enum
>> type that you would use to look up the matching error message.
> Modified. Const char * is passed in.
>
>>> 0008--INI-Change-parse_error-to-use-save_error.patch
>> Nack. This will need to be updated to correspond to the changes for
>> patch 0007.
> Updated to reflect the above patch.
>
>>> 0009--INI-Preparing-for-merging-sections.patch
>> Ack.
>>
> Unchanged
>
>>> 0010--INI-Enhance-value-processing.patch
>> Ack.
>>
> Unchanged
>
>>> 0011--INI-Use-section-line-number.patch
>> Ack.
>>
> Unchanged
>
>>> 0012--INI-Refactor-section-processing.patch
>> Nack. Please fix the formatting of the switch statement. There should be
>> only one level of indent following the case tag.
>>
> Done
>
>>> 0013--INI-Return-error-in-DETECT-mode.patch
>> Ack
>>
> Unchanged
>
>>> 0014--INI-New-test-files-for-section-merge.patch
>> Ack
>>
> Unchanged
>
>>> 0015--INI-Test-DETECT-mode-and-use-new-file.patch
>> Ack
>>
> Unchanged
>>> 0016--INI-Test-for-all-section-merge-modes.patch
>> Nack. Please fix random tabs in the indentation. Otherwise it looks fine.
>>
> Fixed.
>
>
>
> Added another patch, in the new numeration it is patch 15. It is a
> cosmetic change to meet the coding standard.
>
> The rest are deferred in this round.
>
> ------------------------------
>
> I also have a question.
> I use a struct that is not aligned by 8 byte boundary on a 64 bit machine.
>
> pointer = 8
> unsigned = 4
> unsigned = 4
> unsigned = 4
>
> total length is 20 which is padded to 24
>
> I allocate memory for it using sizeof of the structure. Everything is
> fine - no leak. I then set the values of the structure one by one.
> Also in some cases for debugging purposes I print the contents of the
> allocated memory as binary memory byte by byte.
> When I do this with the memory used by this structure valgrind complains
> that I am printing uninitialized memory. And indeed the padding is not
> initialized by me.
> So I can:
> 1) memset memory after allocating it (which I do not like to do
> especially if I can set all the elements to the non zero values anyways
> except for padding)
> 2) Add artificial padding to the structure or a dummy unsigned member
> (seems a bit ugly)
> 3) Ignore the warning (seems wrong)
> 4) Ask your advice (...?)
>
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel

Thanks for review. I do not like memset(). It just hides things. If
it is not initialized coverity and valgrind would not complain and we
will see the issue.

As I said, I don't care sufficiently about it for me to nack it.
However, I strongly disagree that memset() hides things. It's defensive
coding, as it guarantees that even if someone working in another area of
the code adds a new option to the struct, they won't need to remember to
initialize the variable. It's not safe to rely on static analysis tools
to let us know if a variable isn't initialized. Some of them cannot
follow memory through its entire lifecycle.
In the SSSD code, we have a convention that requires us to allocate
memory for structures using talloc_zero(), which internally is a
malloc() plus a memset() to protect against this.
I'd like to know what you think it "hides", though.

There are two reasons that I have in mind.
1) When you add a new structure member you add it for purpose so it
should be initialized to a proper value. In many cases it is 0 but is
some cases it is not. Then you usually add code to do something with
this member (otherwise why you added it in the first place). If you
memset() the whole struct the value is already initialized and if you
forgot to set it to something you would have to spend long time to
figure out what is going on in testing. If you do not memset valgrind
will immediately find that you are using uninitialized value and point
you to the right place helping you to set the proper initial value. This
implies that valgrind is routinely executed which is the case in my dev
practice.
2) Structure can not be aligned so it will be padded. If then you try to
do some operations with the whole blob of data like print it with
debug() function you will access the uninitialized memory. This is not
good and generally you can't assume that the padding will be done in a
same way on different platforms so accessing uninitialized data will be
caught. If you memset you will be able to dump this data but it might be
meaningless as the layout might change from platform to platform.
These are weak arguments but they force the developer to be honest and
thorough so I think it is the "defensive programming" technique. But I
do not force anyone to agree with me.
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

Please see the attached patches. I tried to split the patches
logically
into manageable sets.
Unfortunately I made a minor mistake and I am afraid I will do something
wrong to fix it.
I merged two wrong patches. Fortunately it was three liner with 1 liner
so it is not a big of the deal but I am really scared that I will do
something wrong and loose the work I have done.
So I hope it is Ok to send it as is.
0001--INI-Making-Coverity-happy.patch <- this is the patch I submitted
earlier that I merged by mistake. I was supposed to merge it with patch
25 but picked the wrong one instead.
Patch 25 addresses the real issue found by Coverity as mentioned in
Stephen's review mail but it did not apply cleanly since it relies on
some code from the patches in the middle.
0002--INI-Adding-missing-function-declararion.patch <- this is the
patch that was rejected from the second set sent earlier. Fixed
according to review comment.
0003--BUILD-Allow-trace-per-component.patch <- This patch allows tracing
per component
The following set of patches introduces the merging of sections during
the reading of the file:
0004--INI-New-error-codes-and-messages.patch
0005--INI-New-merge-flags.patch
0006--INI-Add-new-vars-to-parse-structure.patch
0007--INI-Add-save_error-function.patch
0008--INI-Change-parse_error-to-use-save_error.patch
0009--INI-Preparing-for-merging-sections.patch
0010--INI-Enhance-value-processing.patch
0011--INI-Use-section-line-number.patch
0012--INI-Refactor-section-processing.patch
0013--INI-Return-error-in-DETECT-mode.patch
0014--INI-New-test-files-for-section-merge.patch
0015--INI-Test-DETECT-mode-and-use-new-file.patch
0016--INI-Test-for-all-section-merge-modes.patch
Patches related porting of the meta data from old way of doing things to
the new way of doing things:
0017--INI-Separate-close-and-destroy.patch

You should set file_ctx->file to NULL after fclose(file_ctx->file) to
make the if(file_ctx->file) checks work in ini_config_file_close() and
ini_config_file_destroy().
There are tab indents in merge_values_test() and merge_section_test().

You remove metadata from struct ini_cfgfile without removing all
references to metadata in the same patch. You should make clear that
more patches are needed to create a buildable version of libini or
remove all references in this patch.
I wonder is the following is a change of defaults. With the patch the
new file_ctx->file_stats are only set if INI_META_STATS is set while
previously file-ctx>metadata was set unconditionally.

I wonder if it is necessary to return EINVAL if flags == 0. I would say
in this case no checks are requested and EOK could be returned?
I would prefer to copy file_ctx->file_stats.st_mode instead of modifying
it, e.g. you might want to know the file type later on.

0025--INI-Initialize-variables-in-loops.patch <- Coverity issue
addressed. Related to patch 0001.
0026--INI-Exposing-functions.patch <- Make some internal functions reusable
There is also patch 27. It is a piece of new functionality. It is a
preview. Please see the comment before reviewing it.
Do I need to split it into multiple patches or it is Ok as is? It is
pretty big but all changes are in one file and logically related.
The UNIT test is missing so I am not claiming it actually works as
expected.

On Mon, Jan 03, 2011 at 06:12:38PM -0500, Dmitri Pal wrote:
> Please see the attached patches. I tried to split the patches logically
> into manageable sets.
> Unfortunately I made a minor mistake and I am afraid I will do something
> wrong to fix it.
> I merged two wrong patches. Fortunately it was three liner with 1 liner
> so it is not a big of the deal but I am really scared that I will do
> something wrong and loose the work I have done.
> So I hope it is Ok to send it as is.
>
> 0001--INI-Making-Coverity-happy.patch <- this is the patch I submitted
> earlier that I merged by mistake. I was supposed to merge it with patch
> 25 but picked the wrong one instead.
> Patch 25 addresses the real issue found by Coverity as mentioned in
> Stephen's review mail but it did not apply cleanly since it relies on
> some code from the patches in the middle.
>
> 0002--INI-Adding-missing-function-declararion.patch <- this is the
> patch that was rejected from the second set sent earlier. Fixed
> according to review comment.
>
> 0003--BUILD-Allow-trace-per-component.patch <- This patch allows tracing
> per component
>
> The following set of patches introduces the merging of sections during
> the reading of the file:
> 0004--INI-New-error-codes-and-messages.patch
> 0005--INI-New-merge-flags.patch
> 0006--INI-Add-new-vars-to-parse-structure.patch
> 0007--INI-Add-save_error-function.patch
> 0008--INI-Change-parse_error-to-use-save_error.patch
> 0009--INI-Preparing-for-merging-sections.patch
> 0010--INI-Enhance-value-processing.patch
> 0011--INI-Use-section-line-number.patch
> 0012--INI-Refactor-section-processing.patch
> 0013--INI-Return-error-in-DETECT-mode.patch
> 0014--INI-New-test-files-for-section-merge.patch
> 0015--INI-Test-DETECT-mode-and-use-new-file.patch
> 0016--INI-Test-for-all-section-merge-modes.patch
>
> Patches related porting of the meta data from old way of doing things to
> the new way of doing things:
> 0017--INI-Separate-close-and-destroy.patch
>
You should set file_ctx->file to NULL after fclose(file_ctx->file) to
make the if(file_ctx->file) checks work in ini_config_file_close() and
ini_config_file_destroy().
There are tab indents in merge_values_test() and merge_section_test().
> 0018--INI-Function-to-reopen-file.patch
> 0019--INI-Metadata-collection-is-gone.patch
>
You remove metadata from struct ini_cfgfile without removing all
references to metadata in the same patch. You should make clear that
more patches are needed to create a buildable version of libini or
remove all references in this patch.
I wonder is the following is a change of defaults. With the patch the
new file_ctx->file_stats are only set if INI_META_STATS is set while
previously file-ctx>metadata was set unconditionally.
> 0020--INI-Check-access-function.patch
>
I wonder if it is necessary to return EINVAL if flags == 0. I would say
in this case no checks are requested and EOK could be returned?
I would prefer to copy file_ctx->file_stats.st_mode instead of modifying
it, e.g. you might want to know the file type later on.
> 0021--INI-Avoid-double-free.patch <- patch related to 17 (missed check)
>
oops, so you can ignore my comment to 00017, let's see if there is also
a patch for ini_config_file_destroy(). "I might squash this patch into
one of the previous ones." Yes, please.
> 0022--INI-Function-to-check-for-changes.patch
> 0023--INI-Tests-for-access-and-changes.patch
>
Why do you need sleep(1) ?
The man page of system() does not mention that system() sets errno,
please check the return code instead.
> 0024--INI-Rename-error-print-function.patch <- rename error printing
> function for consistency with new interface
>
>
Maybe ini_print_errors() should print a deprecated warning to make
migrations easier.
> 0025--INI-Initialize-variables-in-loops.patch <- Coverity issue
> addressed. Related to patch 0001.
>
> 0026--INI-Exposing-functions.patch <- Make some internal functions reusable
>
> There is also patch 27. It is a piece of new functionality. It is a
> preview. Please see the comment before reviewing it.
> Do I need to split it into multiple patches or it is Ok as is? It is
> pretty big but all changes are in one file and logically related.
> The UNIT test is missing so I am not claiming it actually works as
> expected.
>
I didn't had a look at 0027 so far.
bye,
Sumit

Thank you for the review.
I will address the issues as soon as I find a moment.

> Patches related porting of the meta data from old way of doing
things to
> the new way of doing things:
> 0017--INI-Separate-close-and-destroy.patch
You should set file_ctx->file to NULL after fclose(file_ctx->file) to
make the if(file_ctx->file) checks work in ini_config_file_close() and
ini_config_file_destroy().

> 0019--INI-Metadata-collection-is-gone.patch
You remove metadata from struct ini_cfgfile without removing all
references to metadata in the same patch. You should make clear that
more patches are needed to create a buildable version of libini or
remove all references in this patch.

This is not true. The buildable version is still preserved.
All the code that I am building is not executed yet.
It is a parallel interface. There are currently two interfaces:
ini_config.h and ini_configobj.h
The external code is still suing the ini_config.h.
The new interface that the UT are executing is pointing to the interface
growing in ini_configobj.h.
This metadata functions of the existing ini_config interface are not
affected.
After some thinking I realized that there is too much overhead for doing
the metadata handling the way I did it in the first implementation. The
new interface corrects this but does not affect the old code yet. So the
metadata collection is gone from the new interface not from the old one.
The code compiles fine after this patch and the three patches right
after it take advantage of this change. Patches 18-21 should be
considered together as a block of related patches.

I wonder if it is necessary to return EINVAL if flags == 0. I would
say
in this case no checks are requested and EOK could be returned?

I disagree with
this one. With flags 0 I think it is a noop and might
lead to the wrong assumptions while no operation was actually performed.
IMO passing flags=0 is a codding error in this case and thus should be
reported as such.

> 0021--INI-Avoid-double-free.patch <- patch related to 17
(missed check)
oops, so you can ignore my comment to 00017, let's see if there is also
a patch for ini_config_file_destroy(). "I might squash this patch into
one of the previous ones." Yes, please.

Maybe ini_print_errors() should print a deprecated warning to make
migrations easier.

As I mentioned above there is no impact on the existing interface.
As you notice I made a clone of the existing function with a different
name and made a comment to clean the old function when the interface is
deprecated.
The changes will be made later when we switch to new interface, so it is
premature to print a warning as this function is still used.

>
> There is also patch 27. It is a piece of new functionality. It is a
> preview. Please see the comment before reviewing it.
> Do I need to split it into multiple patches or it is Ok as is? It is
> pretty big but all changes are in one file and logically related.
> The UNIT test is missing so I am not claiming it actually works as
> expected.
I didn't had a look at 0027 so far.

I am going to hold to patch 27 yet. I have other higher priority issues
to address.
Please review and nack/push this ASAP.
I need to clean ding libs and elapi for the audit effort ASAP.
There are too many patches piling to be able to move forward
effectively. I am the bottleneck now and i need to bring all this into a
usable state within a week or so. Please help!
One last thing. Should I switch to the format used in freeipa?
https://fedorahosted.org/freeipa/wiki/PatchFormat
It seems that SSSD/ding-libs does not follow this format. Should it? Not
insisting, just asking.

#0016:
Ack
#0017:
I'm not sure what's the point in checking errno when malloc fails. IIRC the
errno will be always ENOMEM. The same applies to the strndup several lines
below.
The initialization block (new_ctx->.... = NULL) is redundant, you set
everything necessary later and in case of any error, uninitalized values won't
make it out of the function anyway.
#0018:
Ack
#0019:
The return error at the end is misleading, please use return EOK instead
(similar issue is in other patches as well, please change it when you see it).
#0020:
Please change the comment to:
Determines if two file contexts are different by comparing:
I don't like the prototype of ini_config_changed(). Is it necessary to return
special error code? I would perform the check and in case of wrong input, I'd
fall back to the safe option - return true (as in the config file has changed).
In this case no special check is necessary and the code will be more readable.
#0021:
Ack
#0022:
I'm confused. In the patch comment you write that we can't remove it from the
old interface but yet you remove it from the header file. I'd say it should
remain there (and be marked) as well.
#0023:
Ack
#0024:
Ack

On 01/24/2011 12:27 PM, Sumit Bose wrote:
The numeration changed. But the ste is the same. The patches in this
mail are dependent on the patches sent in the previous email on Friday.
>> Patches related porting of the meta data from old way of doing things to
>> the new way of doing things:
>> 0017--INI-Separate-close-and-destroy.patch
>
> You should set file_ctx->file to NULL after fclose(file_ctx->file) to
> make the if(file_ctx->file) checks work in ini_config_file_close() and
> ini_config_file_destroy().
Yes it was fixed in a different patch. Now merged together.
> There are tab indents in merge_values_test() and merge_section_test().
Fixed.
>> 0018--INI-Function-to-reopen-file.patch
Unchanged
>> 0019--INI-Metadata-collection-is-gone.patch
>
> You remove metadata from struct ini_cfgfile without removing all
> references to metadata in the same patch. You should make clear that
> more patches are needed to create a buildable version of libini or
> remove all references in this patch.
This is not true. The buildable version is still preserved.
All the code that I am building is not executed yet.
It is a parallel interface. There are currently two interfaces:
ini_config.h and ini_configobj.h
The external code is still suing the ini_config.h.
The new interface that the UT are executing is pointing to the interface
growing in ini_configobj.h.
This metadata functions of the existing ini_config interface are not
affected.
After some thinking I realized that there is too much overhead for doing
the metadata handling the way I did it in the first implementation. The
new interface corrects this but does not affect the old code yet. So the
metadata collection is gone from the new interface not from the old one.
The code compiles fine after this patch and the three patches right
after it take advantage of this change. Patches 18-21 should be
considered together as a block of related patches.
> I wonder is the following is a change of defaults. With the patch the
> new file_ctx->file_stats are only set if INI_META_STATS is set while
> previously file-ctx>metadata was set unconditionally.
Fixed by memset.
>> 0020--INI-Check-access-function.patch
This is now 19.
> I wonder if it is necessary to return EINVAL if flags == 0. I would say
> in this case no checks are requested and EOK could be returned?
I disagree with this one. With flags 0 I think it is a noop and might
lead to the wrong assumptions while no operation was actually performed.
IMO passing flags=0 is a codding error in this case and thus should be
reported as such.
> I would prefer to copy file_ctx->file_stats.st_mode instead of modifying
> it, e.g. you might want to know the file type later on.
Good catch. Agree. Fixed.
>> 0021--INI-Avoid-double-free.patch <- patch related to 17 (missed check)
>
> oops, so you can ignore my comment to 00017, let's see if there is also
> a patch for ini_config_file_destroy(). "I might squash this patch into
> one of the previous ones." Yes, please.
This one is merged now.
>> 0022--INI-Function-to-check-for-changes.patch
This is now 20 and unchanged.
>> 0023--INI-Tests-for-access-and-changes.patch
This is now 21.
> Why do you need sleep(1) ?
Removed.
> The man page of system() does not mention that system() sets errno,
> please check the return code instead.
Changed to use execlp().
>> 0024--INI-Rename-error-print-function.patch <- rename error printing
>> function for consistency with new interface
This is now 22.
> Maybe ini_print_errors() should print a deprecated warning to make
> migrations easier.
As I mentioned above there is no impact on the existing interface.
As you notice I made a clone of the existing function with a different
name and made a comment to clean the old function when the interface is
deprecated.
The changes will be made later when we switch to new interface, so it is
premature to print a warning as this function is still used.
>> 0025--INI-Initialize-variables-in-loops.patch <- Coverity issue
>> addressed. Related to patch 0001.
This is 23 and merged with another initialization one liner patch.
>> 0026--INI-Exposing-functions.patch <- Make some internal functions
>> reusable
This is 24 and unchanged.
>> There is also patch 27. It is a piece of new functionality. It is a
>> preview. Please see the comment before reviewing it.
>> Do I need to split it into multiple patches or it is Ok as is? It is
>> pretty big but all changes are in one file and logically related.
>> The UNIT test is missing so I am not claiming it actually works as
>> expected.
>
> I didn't had a look at 0027 so far.
I am going to hold to patch 27 yet. I have other higher priority issues
to address.
Please review and nack/push this ASAP.
I need to clean ding libs and elapi for the audit effort ASAP.
There are too many patches piling to be able to move forward
effectively. I am the bottleneck now and i need to bring all this into a
usable state within a week or so. Please help!
One last thing. Should I switch to the format used in freeipa?
https://fedorahosted.org/freeipa/wiki/PatchFormat
It seems that SSSD/ding-libs does not follow this format. Should it? Not
insisting, just asking.

#0016:
Ack
#0017:
I'm not sure what's the point in checking errno when malloc fails. IIRC the
errno will be always ENOMEM. The same applies to the strndup several lines
below.
The initialization block (new_ctx->.... = NULL) is redundant, you set
everything necessary later and in case of any error, uninitalized values won't
make it out of the function anyway.
#0018:
Ack
#0019:
The return error at the end is misleading, please use return EOK instead
(similar issue is in other patches as well, please change it when you see it).
#0020:
Please change the comment to:
Determines if two file contexts are different by comparing:
I don't like the prototype of ini_config_changed(). Is it necessary to return
special error code? I would perform the check and in case of wrong input, I'd
fall back to the safe option - return true (as in the config file has changed).
In this case no special check is necessary and the code will be more readable.
#0021:
Ack
#0022:
I'm confused. In the patch comment you write that we can't remove it from the
old interface but yet you remove it from the header file. I'd say it should
remain there (and be marked) as well.
#0023:
Ack
#0024:
Ack

I am re-sending the whole set.
In #0015 I removed the trailing spaces Stephen noted in the other mail.
The rest 14 are the same.
#0017 - remove the initialization of the filename. Removing other
initialization is unsafe. Agreed on IRC.
Opened a ticket to do trust malloc to return ENOMEM.
https://fedorahosted.org/sssd/ticket/1276
I was told that malloc() by standard must return ENOMEM. Doe this
standard apply to all distros or just Linux?
#0019
Removed error variable and return EOK directly.
#0020
Changed comment.
Removed error variable and return EOK directly.
We agree on IRC that it is OK to keep function signature as is.
#0022
Unchanged.
Explanation:
Some time ago I started building a parallel interface in ini_configobj.h
to the existing public interface that is in ini_config.h So we do not
change the public existing one until the new one is ready and
stabilized. As one of the steps to prepare the new interface I renamed
the function in the new interface to be consistent with the naming
convention. I might provide some compatibility layer when I am ready to
switch from the old interface to the new interface but this is out of
scope of the current patch set.
The rest are same.
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

On 03/26/2012 04:49 AM, Jan Zelený wrote:
> #0016:
> Ack
>
> #0017:
> I'm not sure what's the point in checking errno when malloc fails. IIRC
> the errno will be always ENOMEM. The same applies to the strndup several
> lines below.
>
> The initialization block (new_ctx->.... = NULL) is redundant, you set
> everything necessary later and in case of any error, uninitalized values
> won't make it out of the function anyway.
>
> #0018:
> Ack
>
> #0019:
> The return error at the end is misleading, please use return EOK instead
> (similar issue is in other patches as well, please change it when you see
> it).
>
> #0020:
> Please change the comment to:
> Determines if two file contexts are different by comparing:
>
> I don't like the prototype of ini_config_changed(). Is it necessary to
> return special error code? I would perform the check and in case of
> wrong input, I'd fall back to the safe option - return true (as in the
> config file has changed). In this case no special check is necessary and
> the code will be more readable.
>
> #0021:
> Ack
>
> #0022:
> I'm confused. In the patch comment you write that we can't remove it from
> the old interface but yet you remove it from the header file. I'd say it
> should remain there (and be marked) as well.
>
> #0023:
> Ack
>
> #0024:
> Ack
I am re-sending the whole set.
In #0015 I removed the trailing spaces Stephen noted in the other mail.
The rest 14 are the same.
#0017 - remove the initialization of the filename. Removing other
initialization is unsafe. Agreed on IRC.
Opened a ticket to do trust malloc to return ENOMEM.
https://fedorahosted.org/sssd/ticket/1276
I was told that malloc() by standard must return ENOMEM. Doe this
standard apply to all distros or just Linux?

#0022
Unchanged.
Explanation:
Some time ago I started building a parallel interface in ini_configobj.h
to the existing public interface that is in ini_config.h So we do not
change the public existing one until the new one is ready and
stabilized. As one of the steps to prepare the new interface I renamed
the function in the new interface to be consistent with the naming
convention. I might provide some compatibility layer when I am ready to
switch from the old interface to the new interface but this is out of
scope of the current patch set.

I think that my comment still stands. I understand your explanation but I
still don't understand what's the point in keeping the function when you don't
have its declaration in header file. ABI compatibility? This is the last
question I have regarding the patch set, other than that it's ok.
Thanks
Jan

On 03/26/2012 04:49 AM, Jan Zelený wrote:
> #0016:
> Ack
>
> #0017:
> I'm not sure what's the point in checking errno when malloc fails. IIRC the
> errno will be always ENOMEM. The same applies to the strndup several lines
> below.
>
> The initialization block (new_ctx->.... = NULL) is redundant, you set
> everything necessary later and in case of any error, uninitalized values won't
> make it out of the function anyway.
>
> #0018:
> Ack
>
> #0019:
> The return error at the end is misleading, please use return EOK instead
> (similar issue is in other patches as well, please change it when you see it).
>
> #0020:
> Please change the comment to:
> Determines if two file contexts are different by comparing:
>
> I don't like the prototype of ini_config_changed(). Is it necessary to return
> special error code? I would perform the check and in case of wrong input, I'd
> fall back to the safe option - return true (as in the config file has changed).
> In this case no special check is necessary and the code will be more readable.
>
> #0021:
> Ack
>
> #0022:
> I'm confused. In the patch comment you write that we can't remove it from the
> old interface but yet you remove it from the header file. I'd say it should
> remain there (and be marked) as well.
>
> #0023:
> Ack
>
> #0024:
> Ack
>
I am re-sending the whole set.
In #0015 I removed the trailing spaces Stephen noted in the other mail.
The rest 14 are the same.
#0017 - remove the initialization of the filename. Removing other
initialization is unsafe. Agreed on IRC.
Opened a ticket to do trust malloc to return ENOMEM.
https://fedorahosted.org/sssd/ticket/1276
I was told that malloc() by standard must return ENOMEM. Doe this
standard apply to all distros or just Linux?
#0019
Removed error variable and return EOK directly.
#0020
Changed comment.
Removed error variable and return EOK directly.
We agree on IRC that it is OK to keep function signature as is.
#0022
Unchanged.
Explanation:
Some time ago I started building a parallel interface in ini_configobj.h
to the existing public interface that is in ini_config.h So we do not
change the public existing one until the new one is ready and
stabilized. As one of the steps to prepare the new interface I renamed
the function in the new interface to be consistent with the naming
convention. I might provide some compatibility layer when I am ready to
switch from the old interface to the new interface but this is out of
scope of the current patch set.

Ok the whole set once more:
1) Fixed the trailing spaces on patch 15 - they showed as warning when I
applied the patch in a different repo
2) The problem with permissions that Jan have seen was caused by the
fact that when the repo is created as root the permissions on the files
that are used in tests are different. For now I added a different check.
In future there might be a need for some improvement in this area. I
created a task https://fedorahosted.org/sssd/ticket/1284 to track this
effort.
3) Added two missing cleanup calls to patch 27 as Jan rightly indicated
that they were missing
All the rest is the same.
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

Ok the whole set once more:
1) Fixed the trailing spaces on patch 15 - they showed as warning when I
applied the patch in a different repo
2) The problem with permissions that Jan have seen was caused by the
fact that when the repo is created as root the permissions on the files
that are used in tests are different. For now I added a different check.
In future there might be a need for some improvement in this area. I
created a task https://fedorahosted.org/sssd/ticket/1284 to track this
effort.
3) Added two missing cleanup calls to patch 27 as Jan rightly indicated
that they were missing
All the rest is the same.

I'm not completely comfortable with your solution of the access check. You are
just assuming in the comment that the file was created by root and has
therefore different permissions. But what if it wasn't created by root and
still has these different permissions? I think the test could then let an error
in the code pass through. Please check that the file indeed belongs to root
just to be sure. Also error2 is not necessary IMO, you can re-user error.
This is the last minor change I'm asking for. If you want, I'll do it for you.
Other than that all patches are fine.
Jan

> Ok the whole set once more:
>
> 1) Fixed the trailing spaces on patch 15 - they showed as warning when I
> applied the patch in a different repo
> 2) The problem with permissions that Jan have seen was caused by the
> fact that when the repo is created as root the permissions on the files
> that are used in tests are different. For now I added a different check.
> In future there might be a need for some improvement in this area. I
> created a task https://fedorahosted.org/sssd/ticket/1284 to track this
> effort.
> 3) Added two missing cleanup calls to patch 27 as Jan rightly indicated
> that they were missing
>
> All the rest is the same.
I'm not completely comfortable with your solution of the access check. You are
just assuming in the comment that the file was created by root and has
therefore different permissions. But what if it wasn't created by root and
still has these different permissions? I think the test could then let an error
in the code pass through. Please check that the file indeed belongs to root
just to be sure. Also error2 is not necessary IMO, you can re-user error.
This is the last minor change I'm asking for. If you want, I'll do it for you.
Other than that all patches are fine.

Actually, I think it's more likely that what's happening here is that
root and a casual user have a different default umask on most systems.
So this probably IS a bug in the code. You should be setting the umask
before creating the file (and resetting the umask to its old value
afterwards) in order to guarantee that it is always written with the
proper permissions.

On Tue, 2012-04-03 at 10:08 +0200, Jan Zelený wrote:
>> Ok the whole set once more:
>>
>> 1) Fixed the trailing spaces on patch 15 - they showed as warning when I
>> applied the patch in a different repo
>> 2) The problem with permissions that Jan have seen was caused by the
>> fact that when the repo is created as root the permissions on the files
>> that are used in tests are different. For now I added a different check.
>> In future there might be a need for some improvement in this area. I
>> created a task https://fedorahosted.org/sssd/ticket/1284 to track this
>> effort.
>> 3) Added two missing cleanup calls to patch 27 as Jan rightly indicated
>> that they were missing
>>
>> All the rest is the same.
> I'm not completely comfortable with your solution of the access check. You are
> just assuming in the comment that the file was created by root and has
> therefore different permissions. But what if it wasn't created by root and
> still has these different permissions? I think the test could then let an error
> in the code pass through. Please check that the file indeed belongs to root
> just to be sure. Also error2 is not necessary IMO, you can re-user error.
>
> This is the last minor change I'm asking for. If you want, I'll do it for
you.
> Other than that all patches are fine.
Actually, I think it's more likely that what's happening here is that
root and a casual user have a different default umask on most systems.
So this probably IS a bug in the code. You should be setting the umask
before creating the file (and resetting the umask to its old value
afterwards) in order to guarantee that it is always written with the
proper permissions.

The file is a part of the repo. It is checked into the source control. I
do not have control on its permissions.
I think it is the umask but not during execution but rather during git
clone.
Yes this area needs more thought but IMO in a separate patch. And may be
done differently. For example create a file on the fly and make sure it
has specific permissions. But this is way out of the scope of the
current patch. This is why I opened a separate ticket to handle it.

On 04/03/2012 08:01 AM, Stephen Gallagher wrote:
> On Tue, 2012-04-03 at 10:08 +0200, Jan Zelený wrote:
>>> Ok the whole set once more:
>>>
>>> 1) Fixed the trailing spaces on patch 15 - they showed as warning when
>>> I applied the patch in a different repo
>>> 2) The problem with permissions that Jan have seen was caused by the
>>> fact that when the repo is created as root the permissions on the files
>>> that are used in tests are different. For now I added a different
>>> check. In future there might be a need for some improvement in this
>>> area. I created a task https://fedorahosted.org/sssd/ticket/1284 to
>>> track this effort.
>>> 3) Added two missing cleanup calls to patch 27 as Jan rightly indicated
>>> that they were missing
>>>
>>> All the rest is the same.
>>
>> I'm not completely comfortable with your solution of the access check.
>> You are just assuming in the comment that the file was created by root
>> and has therefore different permissions. But what if it wasn't created
>> by root and still has these different permissions? I think the test
>> could then let an error in the code pass through. Please check that the
>> file indeed belongs to root just to be sure. Also error2 is not
>> necessary IMO, you can re-user error.
>>
>> This is the last minor change I'm asking for. If you want, I'll do it
>> for you. Other than that all patches are fine.
>
> Actually, I think it's more likely that what's happening here is that
> root and a casual user have a different default umask on most systems.
> So this probably IS a bug in the code. You should be setting the umask
> before creating the file (and resetting the umask to its old value
> afterwards) in order to guarantee that it is always written with the
> proper permissions.
The file is a part of the repo. It is checked into the source control. I
do not have control on its permissions.
I think it is the umask but not during execution but rather during git
clone.
Yes this area needs more thought but IMO in a separate patch. And may be
done differently. For example create a file on the fly and make sure it
has specific permissions. But this is way out of the scope of the
current patch. This is why I opened a separate ticket to handle it.

Ok, thanks for clarification. Since the issue is in the test suite and it has
been worked around, I give these patches a green light.
Ack to all
Thanks
Jan

On Thu, 2012-04-05 at 15:54 +0200, Jan Zelený wrote:
> Ok, thanks for clarification. Since the issue is in the test suite and it has
> been worked around, I give these patches a green light.
>
> Ack to all
All 28 patches pushed to ding-libs master.
If any of them should also be pushed to the stable branch, please
provide guidance.