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

Stephen Gallagher sgallagh at redhat.com
Wed Jan 5 19:35:24 UTC 2011


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

On 01/05/2011 02:13 PM, Dmitri Pal wrote:
> 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?
> 

Well, they're "valid", but they're not accomplishing anything at all. In
general, I nack no-op patches :)

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

I still vote nack, as I don't like the approach.


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

Thanks.

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

#define INI_MV1S_OVERWRITE 0x0000
#define INI_MV1S_ERROR     0x0001
#define INI_MV1S_PRESERVE  0x0002
#define INI_MV1S_ALLOW     0x0003
#define INI_MV1S_DETECT    0x0004

I don't like that on a bitwise compare, if you do:
var = INI_MV1S_ALLOW;

then

if (var & INI_MV1S_ERROR) {
}
would be true. This isn't how flags are supposed to work. You're using
these more like an enum. It would be better to define them as such (and
not call them "flags" if that's what you're trying to do.

Calling them flags implies that you would expect to be able to do:

var = INI_MV1S_OVERWRITE | INI_MV1S_OVERWRITE;

And have both of those be active.

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

Right, but memsetting to zero guarantees that everything is intialized.
Like I said, the patch is acked, but it's not my favorite style :)


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

iEYEARECAAYFAk0kx/wACgkQeiVVYja6o6MpYACglWj2DDa3+IkRHdOT+sEa847X
zQgAoJJz7hCY1Kw6yo1zduvHsk7sLPlq
=jtnb
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list