[SSSD] [PATCHES] INI meta data interface patches

Dmitri Pal dpal at redhat.com
Thu Apr 8 23:31:47 UTC 2010


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?
>
>
>   
>>     * 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.
>
>   
>> * 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?
>
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
>
>   
I think I addressed all the issues except trace.
Set of new patches attached.
I noticed that I added "can_free" back again in patch 3. I fixed it in
patch 4.
Addressing issues in 4 patches is not a simple task I should say...

-- 
Thank you,
Dmitri Pal

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-Adding-metadata-interface.patch
Type: text/x-patch
Size: 64168 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100408/429fd2b9/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002--INI-Addinig-content-to-the-metadata.patch
Type: text/x-patch
Size: 8287 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100408/429fd2b9/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003--INI-Resolve-paths-for-reporting-purposes.patch
Type: text/x-patch
Size: 3220 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100408/429fd2b9/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004--INI-Acess-control-and-config-change-checks.patch
Type: text/x-patch
Size: 34431 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100408/429fd2b9/attachment-0003.bin>


More information about the sssd-devel mailing list