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

Jan Cholasta jcholast at redhat.com
Thu Sep 22 12:02:56 UTC 2011


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

-- 
Jan Cholasta




More information about the Freeipa-devel mailing list