Jakub Hrozek wrote:
On 08/25/2009 11:00 PM, Dmitri Pal wrote:
> Please see the patch comment.
> ------------------------------------------------------------------------
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)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(a)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/