On Wed, Feb 26, 2014 at 11:33:10AM +0100, Pavel Březina wrote:
On 02/24/2014 05:39 PM, Jakub Hrozek wrote:
>Hi,
>
>The attached two patches are related to:
>https://fedorahosted.org/sssd/ticket/2257
>
>The first patch is included pretty much for completeness, as I noted
>during development of the unit test, the blob type didn't handle any
>default value.
>
>The second patch directly addresses #2257. The previous code didn't
>handle copying options well if the option was set to zero, because the
>code followed logic like:
> if (oldval) {
> newval = oldval;
> else {
> newval = defval;
> }
>
>The patch implements Sumit's idea to provide a separate function for
>copying default values and amend the generic copy function to only
>create a copy using the current values.
Nack.
I think the approach is all right for this purpose, however have you
considered creating a flag "value_set" instead?
Yes, that was our first idea when we discussed the problem with Sumit.
But then he came up with the argument (which I agree with) that _copy
function should only really copy assigned values, not do a half-copy of
defaults and half of values..hence the split of copy and copy_defaults.
I see the following warnings in your unit test:
Ah, thank you, I didn't see them with clang, but they were reproducable
with GCC. Especially the first warning was bad and perhaps a compiler
bug (I suspect that clang optimizes the strlen for sizeof internally,
but I don't think it should do so with -O0)