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

Stephen Gallagher sgallagh at redhat.com
Wed Jan 5 20:47:16 UTC 2011


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

On 01/05/2011 03:40 PM, Dmitri Pal wrote:
> 
>>
>>>>
>>>>> 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.
> 
> So this is the dead lock becuase I like the approach and do not like
> yours. How we are going to resolve it?
> 

Ultimately, it's your project, not mine. You have commit privilege and
full discretion on when to use it. You asked for my review comments. You
don't have to listen to them :)

>>
>>
>>>>> 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.
> 
> I view it differently. Within the range they are mutually exclusive but
> they are flags since I expect it to be used in a following way:
> 
> var = INI_MS_MERGE  |  INI_MV1S_OVERWRITE  |  INI_MV2S_OVERWRITE;
> 
> Do you have a better way of accomplishing what I am trying to do?
> 
> 

I think this would be better served if var was:

struct ini_struct {
    enum ms_cmd ms;
    enum mv1s_cmd mv1s;
    enum mv2s_cmd mv2s;
}

This would guarantee the mutual exclusivity.

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

iEYEARECAAYFAk0k2NQACgkQeiVVYja6o6OaUwCeNRrcT7woH5/E/NggUXZz2W3r
fzEAnisTfZ+35wBInvt6DgyL1qbsUQz/
=O60K
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list