[SSSD] [PATCH] ELAPI sinks and providers

Jakub Hrozek jhrozek at redhat.com
Fri Aug 28 09:40:57 UTC 2009


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> Thank you for the review.
> My comments are inline.
> 
>> Couple of comments:
>> * Instead of putting libprovider.la into _SOURCES, maybe you wanted to
>> add it to libelapi_la_LIBADD?
>>
> I consulted Steve on this one and showed him the makefile.
> He was Ok with it. But may be you right.

Then it's probably right, it's just something that I noticed.

> I will wait for Steven to take a second look at this one.
> 
>> * in elapi_print_sink_ctx() instead of casting pointer to uint32_t and
>> printing with %X, you should use %p without casting the pointer
>>
> Most of my C is in pre C99 style.  I just did not know about the option.
> I will definitely switch it to %p if it is portable between platforms.
> Are you sure it is portable?
> 

Yes, it is part of C standard.

>> * why are elapi_sink_cfg.required and elapi_sink_cfg.synch defined as
>> uint32_t? They seem to have bool semantics, maybe bool would be more
>> appropriate. Anyhow, using uint32_t to store unsigned int (or unsigned
>> char in case of get_bool_config_value() even though it should not matter
>> here as unsigned char will be smaller than 32 bits) return value might
>> not be correct on all platforms.
> Here is the explanation.
> I started with "bool".
> What happened is that when bool is a part of structure it gets aligned
> on the boundary
> of the word.
> In case of 32bit system on the boundary of the 4 bytes.
> The tree bytes in this case remain uninitialized and valgrind  screams
> and shouts about them.
> So I decided that since the memory is wasted anyways use 4 bytes and
> make valgrind happy.
> This is why I switched from bools to uint32_t.
> I am not aware of any platform where such assignment will be incorrect.
> I would prefer not using bools since they make valgrind  scream.
>> ** the same applies to struct file_prvdr_cfg in file_provider.h, I don't
>> understand using uint32_t to store bool values
> Same reason.
> 
>> ** Not strictly related to the patch but why does
>> get_bool_config_value() return char and not bool?
>>
> Because I do not like "bools"  and situation with valgrind showed that
> they can't be trusted.
> Using unsigned chars and ints is a proven way of expressing bool for
> years so I do not see a problem
> continuing along this path.
> 

OK, I guess I'm too used to use C99 :-)

>> * in elapi_load_lib() why not use snprintf instead of checking the
>> length and then using sprintf() ?
> Another function that I am not used to use.
> I also prefer to check first.
> Why copy strings and figure that they do not fit later if you can check
> before and see ahead if they fit?

OK, snprintf is another C99 novelty, so I guess just another way of
doing the same.

>>
>> * in elapi_sink_loader() instead of using array of names and a switch
>> just to assign the callback, maybe it would be cleaner to use array of {
>> name, ability } tuples  - that way you would just update on one place
>>
> I will open a ticket to improve this part.
> 
>> * in elapi_sink_create(), if the intent is to have the structure zeroed,
>> why not use calloc()? Or zero the structure out with memset?
>>
> First of all I was told that memset is a slow operation and should be
> avoided.
> This is why I initialize only those members that can be cleaned (freed
> or closed like file descriptor for example).
> 
> Also this is another valgrind trick.
> I noticed that if you use malloc+memset or calloc valgrind does not
> catch that memory as initialized.
> So if you later access a member of the structure to check if it is null
> valgring treats it as an uninitialized read.
> With the explicit initialization I can see that all the alloc/free logic
> works correctly by making sure valgrind is happy.
> 
> I do not see any of those comments to be critical (unless of cause
> others do).
> I would prefer opening tickets to improve different areas and work on
> them later and take this patch as is.
> Any concerns or objections?

No, none of this was critical, more like things I noticed reading the code.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkqXpikACgkQHsardTLnvCXB1ACeN/2/MvWP5+a0CltRzfc1+ivG
FDQAoLTJMMWhINMxv2KJSdQeafFWBCfR
=arr6
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list