[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] virsh: rework command parsing



On 09/13/2010 11:03 PM, Eric Blake wrote:
> On 09/13/2010 01:41 AM, Lai Jiangshan wrote:
>> And the usage was changed:
>> old:
>> virsh [options] [commands]
>>
>> new:
>> virsh [options]... [commands string]
>> virsh [options]... [<command>  [command options]...]
> 
> This needs a tweak, otherwise, executing:
> 
> virsh
> 
> ambiguously matches both forms.  In general, you should list the
> shortest possible form first.  Also, it seems like passing multiple
> command strings would be easy to do with this format, such that these
> two are equivalent:
> 
> virsh "define D.xml" "dumpxml D"
> virsh "define D.xml; dumpxml D"
> 
> So, I think it should be:
> 
> virsh [options]... [<command> [command options]...]
> virsh [options]... <commands_string>...
> 
>> Misc changes:
>> 1) support escape '\'
>> 2) a better quoting support, the following commands are now supported:
>>       virsh # dumpxml --"update-cpu" vm1
>>       virsh # dumpxml --update-cpu vm"1"
>> 3) better handling the boolean options, in old code the following
>> commands are equivalent:
>>       virsh # dumpxml --update-cpu=vm1
>>       virsh # dumpxml --update-cpu vm1
>>     after this patch applied, the first one will become illegal.
> 
> Should any of these be split into separate patches?  It's easier to
> review a series of small patches that each do one thing than it is to
> review 350 lines of multiple changes smashed together.
> 
>>
>> Signed-off-by: Lai Jiangshan<laijs cn fujitsu com>
>> ---
>>   virsh.c |  351
>> ++++++++++++++++++++++++++++++----------------------------------
> 
> Therefore, I haven't closely reviewed this patch yet, but just from the
> commit message, I like the direction it is headed in.
> 

I am still waiting for review comments, I will collect comments
and fix the patch for next version.

Thanks, Lai


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]