-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/28/2009 05:40 AM, Jakub Hrozek wrote:
> 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.
While I sincerely appreciate the vote of confidence, I made a mistake.
You are correct that is should be part of _LIBADD, not _SOURCES here.
> 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.
In order to waste less memory, wouldn't a uint8_t be a better choice?
> 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).
memset() is a bad operation to use when dealing with
strings/null-terminated arrays. It's perfectly valid to use on structs
(and advisable, even). The reason it's bad for strings is that it does
two things unnecessarily: 1) it modifies every entry in the array when
it needs only set the first entry to null-termination and 2) it gives a
false sense of security that the string is now guaranteed to be
terminated (which is not the case if you write past the end of the
buffer). On the other hand, its use in a struct is completely reasonable
because (unless there are static buffers in the struct) you are only
writing a minimal set of zeroes.
> 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.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
- --
Stephen Gallagher
RHCE 804006346421761
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora -
http://enigmail.mozdev.org/
iEYEARECAAYFAkqbxtEACgkQeiVVYja6o6PFvACgii8njUoU4foiGvcrS9VlKwHh
fcEAoIzCWqzf3Dg+DFFlqj0b93Qhw4Yl
=iEWk
-----END PGP SIGNATURE-----