Dmitri Pal wrote: > Jakub Hrozek wrote: > >> On 08/25/2009 11:00 PM, Dmitri Pal wrote: >> >>> Please see the patch comment. >>> >> >> >> >>> ------------------------------------------------------------------------ >>> >>> _______________________________________________ >>> sssd-devel mailing list >>> [email protected] >>> 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. > I would have to do some changes in this area anyways in a separate patch because based on on our conversation with Simo I would have to redo the providers anyways a bit. The stderr will be a special variant of the file provider, not a provider by itself. So the code I sent out and the code I am working on now would have to be corrected anyways to allow provider to be used for both cases. > >> * 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 > [email protected] > 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/ _______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
