[SSSD] [INI] Patches for ding-libs: Merging config sections, handling metadata, remaining Coverity issues...

Dmitri Pal dpal at redhat.com
Wed Jan 5 19:13:52 UTC 2011


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.



>
> > 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 at lists.fedorahosted.org
> > https://fedorahosted.org/mailman/listinfo/sssd-devel
>
>

-------------------------

_______________________________________________
sssd-devel mailing list
sssd-devel at 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/




More information about the sssd-devel mailing list