[SSSD] [PATCHES] INI meta data interface patches

Jakub Hrozek jhrozek at redhat.com
Thu Apr 8 15:41:02 UTC 2010


-----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-----



More information about the sssd-devel mailing list