[SSSD] [PATCH] Fix copying default dp_option values

Pavel Březina pbrezina at redhat.com
Wed Feb 26 13:24:44 UTC 2014


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.




More information about the sssd-devel mailing list