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

Dmitri Pal dpal at redhat.com
Fri Mar 16 05:22:28 UTC 2012


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 (...?)


-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/



-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-INI-Fixing-coverity-issues-10078-10079.patch
Type: text/x-patch
Size: 1907 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120316/b3df1795/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-INI-New-error-codes-and-messages.patch
Type: text/x-patch
Size: 4155 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120316/b3df1795/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-INI-New-merge-flags.patch
Type: text/x-patch
Size: 2680 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120316/b3df1795/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-INI-Add-new-vars-to-parse-structure.patch
Type: text/x-patch
Size: 1937 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120316/b3df1795/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-INI-Add-save_error-function.patch
Type: text/x-patch
Size: 1655 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120316/b3df1795/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-INI-Change-parse_error-to-use-save_error.patch
Type: text/x-patch
Size: 2304 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120316/b3df1795/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-INI-Preparing-for-merging-sections.patch
Type: text/x-patch
Size: 6591 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120316/b3df1795/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-INI-Enhance-value-processing.patch
Type: text/x-patch
Size: 10463 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120316/b3df1795/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-INI-Use-section-line-number.patch
Type: text/x-patch
Size: 1037 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120316/b3df1795/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0010-INI-Refactor-section-processing.patch
Type: text/x-patch
Size: 5317 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120316/b3df1795/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0011-INI-Return-error-in-DETECT-mode.patch
Type: text/x-patch
Size: 1183 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120316/b3df1795/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0012-INI-New-test-files-for-section-merge.patch
Type: text/x-patch
Size: 12624 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120316/b3df1795/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0013-INI-Test-DETECT-mode-and-use-new-file.patch
Type: text/x-patch
Size: 3269 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120316/b3df1795/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0014-INI-Test-for-all-section-merge-modes.patch
Type: text/x-patch
Size: 9735 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120316/b3df1795/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0015-INI-Fix-indentention-in-the-switch-statement.patch
Type: text/x-patch
Size: 1659 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120316/b3df1795/attachment-0014.bin>


More information about the sssd-devel mailing list