Stephen Gallagher wrote: > 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. Well next patch will correct it. I already have it the code
> > >> 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? It is the structure that would be allocated once per instance of ELAPI. I do not see it being a big deal. And the compiler would try to align things in the struct anyways. I will keep it in mind in cases where memory is allocated frequently and large volumes. [snip] > 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. > I will check with valgrind if my observation about memset and calloc is false I will revert to those where it makes sense. I think it makes sense to wait for the next patch and review the second one first and then push them together. Thanks, Dmitri _______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
