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