On Wed, Feb 26, 2014 at 02:24:44PM +0100, Pavel Březina wrote:
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.
Pushed to master and sssd-1-11