Re: [libvirt] [PATCH 12/17] Improve vcpupin to support hypervisorpin dynamically.

On 08/03/2012 12:36 AM, Hu Tao wrote: Dan's comment of s/hypervisor/emulator/ applies here as well. Why did you have to change this example invocation? If you broke back-compat with existing clients, then we need to rethink how to add the new functionality into virsh. Since this was a negative test (where the command used to fail), if the command now succeeds where it used to fail, and adding '--vcpu' preserves the negative test aspect, I could understand it; but that doesn't appear to be the case here. Changing an error message is okay, though. Again, you shouldn't be needing to modify this test. This is the broken change - you can't require existing users to start typing --vcpu. bool options cannot require an option, so using VSH_OFLAG_REQ_OPT is wrong here (hmm, virsh.c's self-check validation should probably be erroring out on this combination). I think what you want to end up with is: vcpupin dom => query ALL pinnings vcpupin dom --emulator => query just the emulator pinning vcpupin dom [--vcpu] number => query just vcpuN pinning vcpupin dom --emulator cpulist => set the emulator pinning to cpulist vcpupin dom [--vcpu] number cpulist => set the vcpuN pinning to cpulist Or in virsh.pod syntax: vcpupin <domain> [ { [--vcpu] <number> | <--emulator> } [<cpulist>] ] [--config] [--live] [--current] such that either --emulator or an integer but not both must be present if cpulist is also present; and that an integer can optionally be prefixed with --vcpu. There's no way to write that directly in opts_vcpupin, so it requires listing both options as optional, then manual checks in the function body that exactly one of the two options was present. Hmm, I see a potential gotcha: the parser would treat: vcpupin dom --emulator 0 as assigning '0' to --vcpu, instead of to --cpulist, so users of --emulator would always have to spell out: vcpupin dom --emulator --cpulist 0 but since that's a new command, at least we wouldn't be breaking back-compat. Maybe a solution would be to take --vcpu -1 as the special case of requesting the emulator pinning, instead of adding a new flag --emulator. That makes sense if we do the same overloading at the libvirt API level. Or maybe the solution is to add a new command 'virsh emulatorpin' for emulator pinning, instead of trying to overload 'virsh vcpupin', especially, since you have proposed separate libvirt APIs. That does mean you can't list all pinnings in one command, but that's not too tough of a restriction. I'm still thinking about the best way to resolve this UI issue, and I think part of the answer is determined by the API answer in 9/17. We should probably name this vshPrintPinInfo, to keep our helper functions in the virsh namespace. For patch review purposes, it would help to split this into two patches - one that refactors code to introduce this new function and call it from the old location but has no semantic changes, and then one to add the new options and/or command and becomes a second client of the helper function. Why nhyper? There is only one emulator pinning value. reads better as: --hypervisor and --vcpu cannot both be specified Why >= 0? Either it is 0 (no pinning known) or 1 (exactly one pinning known). You are introducing a regression on the spelling of 'I<domain>'. --hypervisor is a bool, so it should be listed as <--hypervisor>. See my above attempt at describing this virsh.pod layout using {} to indicate mutual exclusion, if we decide to overload this command rather than adding a new one. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org