On Tue, Aug 27, 2013 at 04:48:55PM -0400, Stephen Gallagher wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/27/2013 04:42 PM, Simo Sorce wrote:
> On Tue, 2013-08-27 at 15:22 -0400, Simo Sorce wrote:
>> On Tue, 2013-08-27 at 14:05 -0400, Stephen Gallagher wrote:
>>> Patch 0002: Nack
>>>
>>> As discussed on IRC, please split out the patch that removes
>>> the contrib patch into a separate patch in the set and modify
>>> it so that it reverts to using the built-in defaults (which
>>> means updating the sssd.spec.in as well).
>>
>> Ah I had all in one patch, splitting ...
>>
>>> In sss_get_system_ccname_template(), you fail to check the
>>> result of the talloc_strdup(). Also, by convention you should
>>> have an explicit ret = EOK in the success case just before the
>>> done: label.
>>
>> It was actually intentional, but your comment made me think that
>> we probably want to fail if the strdup fails, changed.
>>
>>> Wouldn't it make more sense to return ENOENT than a NULL
>>> ccname? from sss_get_system_ccname_template()? It's not
>>> critical, but we tend to use that approach in the rest of the
>>> code. It would be more clear in the code what you're doing here
>>> too. I had to read it again after reading the consuming
>>> function to understand it properly.
>>
>> I was a bit doubtful because ENOENT always result ina "File not
>> found error" which is quite misleading. So I added a new sssd
>> error of ERR_NOT_FOUND, hpe this is ok.
>
> And now with the right patches.
Ack to all three.
Pushed to master and sssd-1-10