[SSSD] [PATCH] Make the replace-space functionality usable for other responders

Pavel Březina pbrezina at redhat.com
Wed Aug 13 12:41:49 UTC 2014


On 08/13/2014 01:34 PM, Jakub Hrozek wrote:
> On Mon, Aug 11, 2014 at 05:21:27PM +0200, Lukas Slebodnik wrote:
>> On (11/08/14 16:21), Michal Židek wrote:
>>> On 08/11/2014 02:25 PM, Pavel Březina wrote:
>>>> On 08/11/2014 09:54 AM, Jakub Hrozek wrote:
>>>>> On Mon, Aug 11, 2014 at 09:12:24AM +0200, Lukas Slebodnik wrote:
>>>>>> It is your decision. You can:
>>>>>>      a) fix problem in function ifp_user_get_attr_search
>>>>>>      b) remove changes in IFP responder.
>>>>>>
>>>>>> LS
>>>>>
>>>>> Thanks for the quick review. I would prefer to keep the ifp changes in
>>>>> -- I don't realistically expect anyone being bitten by spaces when using
>>>>> InfoPipe, but it's more about being consistent. When we say we want to
>>>>> replace the spaces, we should replace them everywhere..
>>>>
>>>> Patch 1:
>>>> Ack.
>>>>
>>>> Patch 2:
>>>> Man page says "override_space", but you renamed the option to
>>>> "override_default_space". Actually I like the first name better, can you
>>>> switch it back? Or what is the reason for using "default" word?
>
> That was a mistake that crept in when I was reshuffling the patches
> about Lukas' changes. Thanks for catching it!
>
>>>>
>>>> Patch 3:
>>>> Ack.
>>>>
>>>> However, I'm not really sure it is a good idea to allow both logins.
>>>> Wouldn't it be better to allow only one depending whether the option is
>>>> set or not?
>
> I don't see the benefit, sorry.
>
>>>>
>>>> Patch 4:
>>>> Ack.
>>>>
>>>> Patch 5:
>>>> Ack.
>>>>
>>>> Path 6:
>>>> Ack.
>>>>
>>>> The code is good, however, I still see the problem that the
>>>> transformation is not symmetrical. If an input contains the replacement
>>>> character, we cannot perform reverse transformation correctly. Either we
>>>> have to very loudly warn that the character must be unused all over the
>>>> database or fix it.
>>>
>>> We should properly document that the character can not be used
>>> in the database.
>>> There is no way to properly "fix" this issue. For example there
>>> is no way to fix the following:
>> +1
>>
>> LS
>
> I agree as well and I amended the man page hunk in the second patch.
>
> Thank you for the review.

Ok, I'm fine with that. Ack.

Just fix the option name also in commit message prior pushing please.




More information about the sssd-devel mailing list