Re: [PATCH] virsh: rework command parsing

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

 



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@xxxxxxxxxxxxxx>
---
  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.

--
Eric Blake   eblake@xxxxxxxxxx    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]