[SSSD] [PATCH] ELAPI sinks and providers

Dmitri Pal dpal at redhat.com
Thu Aug 27 21:15:40 UTC 2009


Dmitri Pal wrote:
> Jakub Hrozek wrote:
>   
>> On 08/25/2009 11:00 PM, Dmitri Pal wrote:
>>     
>>> Please see the patch comment.
>>>       
>>
>>
>>     
>>> ------------------------------------------------------------------------
>>>       
>>> _______________________________________________
>>> sssd-devel mailing list
>>> sssd-devel at lists.fedorahosted.org
>>> https://fedorahosted.org/mailman/listinfo/sssd-devel
>>>       
> 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.
> 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?
>
>   
>> * 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.
>
>   
>> * 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?
>   
>> * 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.
>   
I would have to do some changes in this area anyways in a separate patch
because based on
on our conversation with Simo I would have to redo the providers anyways
a bit.
The stderr will be a special variant of the file provider, not a
provider by itself.
So the code I sent out and the code I am working on now would have to be
corrected anyways to allow provider to be used for both cases.
>   
>> * 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?
>
>   
>>     Jakub
>>     
> _______________________________________________
> 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