-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 04/06/2010 11:33 PM, Dmitri Pal wrote:
Dmitri Pal wrote:
> Steve saw a crash on 64-bit system and pointed me to the right direction.
> Here is the correcting patch.
> Worked fine of 32-bit...
>
>
Forgot to explicitly mention that this patch is a correcting one and
should be applied on top of the previous ones.
> Thanks
> Dmitri
>
> Dmitri Pal wrote:
>
>> Hello,
>>
>> The following patches introduce the meta data interface as it was
>> discussed earlier on the list.
>> The whole work is split into 4 patches.
>> It is done just for simplicity of the review.
>> Each of the patches applies and runs on top of previous one.
>> As I developed I saw some issues with the code so I corrected them
>> (mostly in the last patch).
>> The names of the patches speck for themselves.
>> The comments in the patches are pretty thorough.
>>
>> Tickets addressed by the effort (tickets are not updated yet with some
>> comments about the alternative ways the patches solve the problem
>> expressed in the ticket):
>>
https://fedorahosted.org/sssd/ticket/405 <- interfaces now allow the
>> caller to do it.
>>
https://fedorahosted.org/sssd/ticket/442 <- done
>>
https://fedorahosted.org/sssd/ticket/173 <- done differently but now
>> available to the caller
>>
https://fedorahosted.org/sssd/ticket/82 <-security checks interfaces are
>> added. The recommended approach is presented in the unit test.
>>
https://fedorahosted.org/sssd/ticket/140 <- the approach changed. This
>> is the best we can do at the moment. But we can enhance meta data and
>> add other checks as the need emerges.
>>
>> It still does not fix the lines collection. It just turns off the
>> creation of it.
>> I just do not know what to do with the lines collcetion at the moment.
>> It will be needed for the grammar validation but until this part is
>> built I am not sure which format for it is the best.
>> So I know it will be needed but it is not needed now. Thus it is ifdefed
>> at the moment.
>>
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
* 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?
=== 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
* errno should be zeroed before fstat()
=== 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..
* 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.
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()
collect_metadata():
* do not cast UID and GID to "%d/int" but rather "%lu/unsigned
long"
similarly, time_t and ino_t are usually defined as unsigned long.
This would require get_checked_item() (and possibly other places
in the code) not to use get_int_config_value()
* CONVERSION_BUFFER is probably going to be enough, but using sprintf
instead snprintf makes me nervous :-)
config_changed():
* if (value[i][0] != value[i][0]) is almost certainly a bug
* there is some debugging code left in the unit tests:
(/* Debugging start ==> */)
=== 0005 ===
Since the patch it corrects was not pushed yet, I think it makes sense
to squash into the previous ones.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora -
http://enigmail.mozdev.org/
iEYEARECAAYFAku9+Q4ACgkQHsardTLnvCUCUACcCBSg6XFoXB3EQ1yRKGw1IWyI
iKEAmgLAG5b51QkAUJEnkGPXB62xfb+n
=tm9L
-----END PGP SIGNATURE-----