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

Dmitri Pal dpal at redhat.com
Mon Mar 19 23:01:29 UTC 2012


On 03/19/2012 04:39 PM, Stephen Gallagher wrote:
> Patch 0001: Ack
>
> Patch 0002: Ack
>
> Patch 0003: Ack
>
> Patch 0004: Ack, but it would be more efficient (maintenance-wise) to
> just memset() the struct instead of setting the individual elements to
> zero.
>
> Patch 0005: Ack
>
> Patch 0006: Ack
>
> Patch 0007: Ack
>
> Patch 0008: Ack
>
> Patch 0009: Ack
>
> Patch 0010: Ack
>
> Patch 0011: Ack
>
> Patch 0012: Ack
>
> Patch 0013: Ack
>
> Patch 0014: Ack
>
> Patch 0015: Nack (minor). I can fix this when I push, but please don't
> include trailing spaces after the case directives.
>

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 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/



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120319/2aa45ac4/attachment.html>


More information about the sssd-devel mailing list