On 09/05/2013 03:09 PM, Simo Sorce wrote:
On Thu, 2013-09-05 at 10:49 +0200, Pavel Březina wrote:
> On 09/04/2013 09:12 PM, Simo Sorce wrote:
>> On Wed, 2013-09-04 at 17:06 +0200, Sumit Bose wrote:
>>> On Tue, Sep 03, 2013 at 02:07:29PM -0400, Simo Sorce wrote:
>>>> On Tue, 2013-09-03 at 15:25 +0200, Pavel Březina wrote:
>>>>> On 09/03/2013 01:27 PM, Sumit Bose wrote:
>>>>>> Hi,
>>>>>>
>>>>>> while looking at expand_ccname_template() becasue of shadowing
rewind()
>>>>>> I realized that there a some issues with some of the new
krb5.conf
>>>>>> templates. This patch fixes them and adds some tests to avoid
similar
>>>>>> issues in the future.
>>>>>>
>>>>>> There is one change in behaviour. If the name in the %{} braces
does not
>>>>>> match any of the known krb5.conf templates for UNIX the new code
returns
>>>>>> an error while the old just returned something, which in most
case will
>>>>>> not be the original input. Please tell me if you prefer the
original
>>>>>> input in this case so that I can change the patch accordingly.
>>>>>
>>>>> I think we should expand the parameters we can, i.e. uid and euid
and
>>>>> leave everything else intact (i.e. don't even check ${LIBDIR}
etc.). Ad
>>>>> absurdum: for what we know, users can ran custom build that supports
>>>>> other variables...
>>>>
>>>> True, I was looking at this code yesterday again as I am hadling 2071
>>>> and I was thinking the same, we should just ignore and leave unchanged
>>>> %{xyz} patterns we do not understand, so that if libkrb5 adds something
>>>> the admin can use we do not have to explicitly support it.
>>>>
>>>>> But if you don't agree, I'll ack this patch.
>>>>
>>>> No, let's nack, and if you all do not mind I can come up with an
>>>> alternative patch that does what I describe above.
>>>
>>> Whatever you prefer, but I can do the needed changes as well. If you do
>>> them, please do not forget to add tests for the cases which are
>>> currently not covered.
>>
>> I opened bug #2076 to track this issue and attached find a different
>> approach with testes against current master.
>>
>> Simo.
>
> Nack.
>
> krb5_utils-tests.c: In function ‘test_krb5_style_expansion’:
> krb5_utils-tests.c:679:9: error: unused variable ‘ret’
>
> It looks good otherwise.
> Can we also include Sumit's tests?
Sumit's tests consider any 'unknown' expansion variable as invalid, so
they would fail.
Attached new version w/o ret and rebased on current master.
Simo.
OK then. Ack.