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

Martin Kosek mkosek at redhat.com
Thu Sep 22 11:27:15 UTC 2011


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




More information about the Freeipa-devel mailing list