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(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
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/