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

Stephen Gallagher sgallagh at redhat.com
Wed Jan 5 18:19:25 UTC 2011


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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.


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

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.

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

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

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

> 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


- -- 
Stephen Gallagher
RHCE 804006346421761

Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk0kti0ACgkQeiVVYja6o6MBiQCfbdwDxH4aFcyXRgmVUSRbGQX9
DzEAoLIGl38t/WUeht0BWQNvef6fMReC
=FIcs
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-INI-Added-missing-initialization.patch
Type: text/x-patch
Size: 779 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110105/75bde873/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-INI-Added-missing-initialization.patch.sig
Type: application/pgp-signature
Size: 72 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110105/75bde873/attachment.sig>


More information about the sssd-devel mailing list