[SSSD] [PATCHES] INI meta data interface patches

Stephen Gallagher sgallagh at redhat.com
Thu Apr 8 17:35:54 UTC 2010


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

On 04/08/2010 01:28 PM, Dmitri Pal wrote:
> Jakub,
> 
> Thanks for the review.
> Comments inline.
>>
>> The patches are big and I'd like to take another look at them, but
>> here's my comments after first reading:
>>
>> === 0001 ====
>> config_from_fd_metadata():
>>     * can_free is not used anywhere
> 
> Yes I will remove it.
> 
>>     * I don't understand the semantics of save_error.
>>       If the creation of the error collection in prepare_metadata fails,
>>       config_from_fd_metadata fails anyway.. Is it OK to check just
>>       for metaflags & INI_META_SEC_ERROR_FLAG and get rid of save_error?
>>
> 
> It takes care of the case when the flags are provided but the metadata
> pointer is actually NULL.
> So the save_flag is set only if
> * metadata is not null
> * we managed to create an object
> * the flag to save error is set
> All three are checked within the prepare_metadata function so it was
> reasonable not to duplicate the check again.
> 
> 
> 
>>
>> === 0002 ====
>> collect_metadata():
>>     * in SSSD, we usually put [out] parameters at the end of
>>       the parameter list..not sure if that is a rule that applies
>>       to the ini/collection libraries
> Yes, I think it is my fault and have been using this style for all APIs
> I was implementing.
> I do not know if it is worth refactoring now, but if team says yes, sure
> I would do it.
> It will probably affect collection, ini, ref_array and anything that
> calls it.
> Do we want to go this rout?

No, it's not worth refactoring existing interfaces. However, we should
probably make sure this policy is in the coding guideline and make sure
we follow it for all new interfaces (like this one).

> 
> 
>>     * errno should be zeroed before fstat()
>>
> Good catch.
> 
> 
>> === 0003 ===
>> ACK
>>
>> === 0004 ===
>> get_ullong_config_value():
>>     * The s/get_long/get_ullong/ fix is good one, but I think the
>>       TRACE_FLOW macro might print incorrect (truncated) values when
>>       we cast unsigned long long to long..
> 
> Yes. But I do not see it as a big issue at least at the moment.
> I think we should change the TRACE macros to use ullong for all.
> I will open a separate ticket for this.

That seems sufficient to me for now.

> 
>>
>> * INI_ACCESS_CHECK_UID, INI_ACCESS_CHECK_GID - the description says
>>   if these are 0, we assume success. How do we test for root's uid/gid?
>>   I think -1 would be much better here.
> 
> I think you are right and the check for "if the file is owned by root"
> is missing.
> I will just need to change the check here. The uid_t is unsigned so -1
> does not make sense.
> 
> 
>>
>> config_access_check():
>>   * I think I already asked this but can you remind me why are data
>>     types such as uid_t, gid_t or mode_t not used in ini/collection
>>     code? Especially in external API like config_access_check()
> 
> Because the collection turns them to the uint32_t/int32_t internally
> anyways.
> If you feel that we still need to use these types in the parameters and
> convert them
> internally I can make this change. I just did not see a big value since the
> values the app will check would probably be numeric constants anyways.
> Am I wrong about that?
> 
> 
>>
>> collect_metadata():
>>    * do not cast UID and GID to "%d/int" but rather "%lu/unsigned long"
> 
> Yes I will fix that. I thought they are signed but we checked with Steve and
> it turns out they actually are unsigned.
> 
>>      similarly, time_t and ino_t are usually defined as unsigned long.
> 
> I will check the ino_t but for time_t I was pretty sure it used to be
> signed.
> I will check.
> 
> 
>>      This would require get_checked_item() (and possibly other places
>>      in the code) not to use get_int_config_value()
> 
> it will be unsigned_long then.
> 
>>    * CONVERSION_BUFFER is probably going to be enough, but using sprintf
>>      instead snprintf makes me nervous :-)
> 
> Ok.
> 
>>
>> config_changed():
>>    * if (value[i][0] != value[i][0]) is almost certainly a bug
>>
> I will look into that.
> 
> 
>> * there is some debugging code left in the unit tests:
>>    (/* Debugging start ==> */)
>>
> 
> The whole function is written as a sample code
> so this comment is intentional to indicate that this portion
> of the code is for debugging and should not be cut&paste
> into the actual implementation.
> 
> 
>> === 0005 ===
>> Since the patch it corrects was not pushed yet, I think it makes sense
>> to squash into the previous ones.
> Yes I already did that.
> So all the changes should be squashed into the 4 th patch too or you
> want them as a separate patch?
> 

Please squash the changes into the patch they apply to.

> 
> _______________________________________________
> 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.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAku+E/oACgkQeiVVYja6o6MaDACcDGvFpCxx98YhdN1Sf9sucKuj
6u0AoLD7roqAMMWJxoU//QTxyByNxeza
=Upe1
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list