Thanks for the review.
The patches are big and I'd like to take another look at them, but
here's my comments after first reading:
=== 0001 ====
* 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 ====
* 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
Do we want to go this rout?
* errno should be zeroed before fstat()
=== 0003 ===
=== 0004 ===
* 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"
I will just need to change the check here. The uid_t is unsigned so -1
does not make sense.
* 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
If you feel that we still need to use these types in the parameters and
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?
* do not cast UID and GID to "%d/int" but rather "%lu/unsigned
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
I will check the ino_t but for time_t I was pretty sure it used to be
I will check.
This would require get_checked_item() (and possibly other
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
instead snprintf makes me nervous :-)
* if (value[i] != value[i]) 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
Engineering Manager IPA project,
Red Hat Inc.
Looking to carve out IT costs?