On 02/26/2014 02:12 PM, Jakub Hrozek wrote:
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.
OK, this sounds reasonable.
>
> 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)
Ack to both.