[SSSD] [PATCH] ELAPI sinks and providers

Dmitri Pal dpal at redhat.com
Mon Aug 31 13:31:41 UTC 2009


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



More information about the sssd-devel mailing list