[SSSD] [PATCH] ELAPI sinks and providers

Dmitri Pal dpal at redhat.com
Thu Aug 27 17:59:22 UTC 2009


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.

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