[Freeipa-devel] [PATCH] Fix File parameter validation when prompting.

[John Dennis]

On 01/28/2010 06:56 PM, Jason Gerard DeRose wrote:
> On Wed, 2010-01-27 at 17:53 +0100, Pavel Zuna wrote:
>> cli.prompt_interactively now loads files before validating the parameter value.
>> It also populates a list of already loaded files, so that cli.load_files knows
>> when a parameter already contains the file contents.
>>
>> Fix #557163
>>
>> Pavel
>
> ack.
>
> This looks reasonable to me, but I'd really like you to add some tests
> for this, especially testing that it works correctly for a command with
> multiple File params.
>
> Rob and John, do you see any problems with this approach?  Does this
> address the needs of the cert plugins?

nck

Maybe I'm reading the code wrong but it seems like there are cases where 
the already_loaded_files list is not updated. If load_files() is called 
then the file is loaded but already_loaded_files is not updated to 
reflect that. Shouldn't the function load_file() update the 
already_loaded_files list to assure already_loaded_files is always in 
sync? Also, wouldn't already_loaded_files be better implemented as a set 
rather than a list?

Aside from the above state consistency issue I think it fixes the 
problem, but I'll be honest I'm not in love with it because it's kind of 
a hack to get around deficiencies in the command parameter framework. I 
always get nervous when I see special case exceptions surgically 
inserted into select pieces of code rather than conforming to a regular 
and consistent framework.

Also this only works because the cli object is short lived and single 
use. If that assumption is ever violated the persistent state in the 
object will present a consistency problem.

Bottom line, I'll ack it if the first issue is resolved or Pavel 
explains why load_files() can't produce an inconsistent state.

-- 
John Dennis <jdennis at redhat.com>

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/