[Freeipa-devel] [PATCH] 44 Fix parameter validation

Jan Cholasta jcholast at redhat.com
Mon Sep 26 15:13:32 UTC 2011


On 26.9.2011 14:18, Martin Kosek wrote:
> On Mon, 2011-09-26 at 11:26 +0200, Jan Cholasta wrote:
>> On 23.9.2011 09:00, Martin Kosek wrote:
>>> On Thu, 2011-09-22 at 14:02 +0200, Jan Cholasta wrote:
>>>> On 22.9.2011 13:27, Martin Kosek wrote:
>>>>> On Wed, 2011-09-21 at 15:31 -0400, Rob Crittenden wrote:
>>>>>> Jan Cholasta wrote:
>>>>>>> On 25.8.2011 18:21, Jan Cholasta wrote:
>>>>>>>> What this patch does:
>>>>>>>>
>>>>>>>> * Make sure arguments are validated and default values are filled in
>>>>>>>> before calling a command.
>>>>>>>> * Add new parameter flag "validate_search" to force validation on search
>>>>>>>> arguments.
>>>>>>>> * Fix validation of IP network parameters in the DNS plugin.
>>>>>>>>
>>>>>>>> https://fedorahosted.org/freeipa/ticket/1627
>>>>>>>>
>>>>>>>> Honza
>>>>>>>>
>>>>>>>
>>>>>>> Redone the patch and split it to 3 parts:
>>>>>>
>>>>>> I haven't tested these yet, these comments are just from reading the
>>>>>> patches.
>>>>>>
>>>>>>>
>>>>>>> * [PATCH 46] Add IP address and IP network parameter types
>>>>>>> Adds two new parameter types, IPAddress and IPNetwork (which replaces
>>>>>>> the validate_search flag, as it was just a hack).
>>>>>>
>>>>>> A param type looks like the way to go. There are other IPaddress
>>>>>> parameters, such in host, should this be expanded to cover that?
>>>>>>
>>>>>> Not sure about replacing List with Str types. It may make no difference
>>>>>> since I'm not sure how a List could be passed for some of these. Can you
>>>>>> make sure there is no possibility that on the wire someone couldn't pass
>>>>>> these as a list and actually do something reasonable in the server? I
>>>>>> suspect they can't but this is a rather major datatype change.
>>>>>>
>>>>>>>
>>>>>>> * [PATCH 44] Fix parameter validation
>>>>>>> Changes Command.get_default so that default_from parameters are
>>>>>>> validated before they are used to create the default value.
>>>>>>
>>>>>> Just a heads-up but this will conflict big time with my password patch.
>>>>>>
>>>>>> It throws away the ordering that the parameters had. This could impact
>>>>>> the order in which interactive prompting happens. Did you verify that it
>>>>>> still works sanely?
>>>>>>
>>>>>>> * [PATCH 47] Remove create_default
>>>>>>> Removes create_default, as it does exactly the same thing as
>>>>>>> default_from, but without the advantage of knowing what parameters are
>>>>>>> used to create the default value. All uses of create_default are
>>>>>>> replaced by default_from with no arguments, because that's all
>>>>>>> create_default is currently used for in IPA.
>>>>>>
>>>>>> I'm not sure why you want to remove this, is it causing problems with
>>>>>> validation?
>>>>>>
>>>>>> In general the patch comments need more details. This patch e-mail has
>>>>>> more information on what each patch does than the patches themselves,
>>>>>> including lacking ticket info.
>>>>>>
>>>>>> rob
>>>>>>
>>>>>
>>>>> Hm, by the way I am not very excited about adopting this sort of changes
>>>>> to framework that close to the 6.2 rebase. Are we sure enough that this
>>>>> won't cause any collateral damage?
>>>>>
>>>>> If possible, I would prefer to integrate it to 3.0 branch so that we
>>>>> have enough time to validate it and fix bugs.
>>>>>
>>>>> Martin
>>>>>
>>>>
>>>> I concur.
>>>>
>>>> Honza
>>>>
>>>
>>> Ok. I created a Trac ticket for 3.0 branch to do this improvement.
>>>
>>> https://fedorahosted.org/freeipa/ticket/1847
>>>
>>> As for 2.1.2 bugfix release, lets do just the "stupid" safe fix.
>>>
>>> Martin
>>>
>>
>> Stupid safe fix attached (obviously it's ipa-2-1 only).
>>
>> Honza
>>
>
> Works fine. Just one nit - I would suggest adding a comment what this
> hack does. Its not clear just from the code.
>
> Martin
>

Added comments.

Honza

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-50.1-validate-dnszone-name_from_ip.patch
Type: text/x-patch
Size: 2886 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20110926/0c691fd4/attachment.bin>


More information about the Freeipa-devel mailing list