[SSSD] [PATCHES] INI meta data interface patches

Dmitri Pal dpal at redhat.com
Thu Apr 8 17:28:10 UTC 2010


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

-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/




More information about the sssd-devel mailing list