On 12/18/2015 10:45 AM, John Ferlan wrote:
v1: http://www.redhat.com/archives/libvir-list/2015-December/msg00731.html
Changes over v1:
1. Insert patch 1 to convert already pushed VSH_POOL into VIRSH_POOL since that was the review comment from this patch series
2. Insert patch 2 to move the POOL_OPT_COMMON to virsh.h for later patch reuse.
3. Use VIRSH_* instead of VSH_* for patches 1-8 (now 3-10)
4. Add usage of common domain for virsh-domain-monitor.c and virsh-snapshot.c (patches 11-12)
5. Add common macros for "network" and "interface" (patches 13-14).
NOTE: I figure to let this perculate for a bit as I'll assume there may be varying opinions on this... Also, the next couple of weeks heavy on people perhaps paying not paying close attention to the list.
I'm a bit conflicted. On one hand, it makes for less bulk in the code and assures consistency when the same option is used by multiple commands. On the other hand, as Peter said in a response to the original patch, it obscures things behind macros which can lead to confusion (and extra time backtracking). If you're looking for a final "vote" from me, I guess I'd give it +1 (brevity wins this time), with these comments: 1) I assume that make check and make syntax-check have been run for each patch. :-) 2) I agree with using "VIRSH_..." instead of "VSH_..." (I dislike the shortening of virsh to vsh - doing that just for 2 characters? What is this, twitter? :-P) 3) I haven't looked at how is meshes with consistency of other macro names in virsh*, but it would make more sense to me if these were named VIRSH_COMMON_OPT_BLAH instead of VIRSH_BLAH_OPT_COMMON It reads better, and sticks the difference out at the end where it is more easily separated from the "common common" part.
John Ferlan (14): virsh: Covert VSH_POOL_ macro to VIRSH_POOL_ virsh: Move VIRSH_POOL_OPT_COMMON to virsh.h virsh: Create macro for common "domain" option virsh: Create macro for common "persistent" option virsh: Create macro for common "config" option virsh: Create macro for common "live" option virsh: Create macro for common "current" option virsh: Create macro for common "file" option virsh: Create macros for common "pool" options virsh: Create macros for common "vol" options virsh: Have domain-monitor use common "domain" option virsh: have snapshot use common "domain" option virsh: Create macro for common "network" option virsh: Create macro for common "interface" option
po/POTFILES.in | 1 + tools/virsh-domain-monitor.c | 77 +--- tools/virsh-domain.c | 911 +++++++++---------------------------------- tools/virsh-interface.c | 37 +- tools/virsh-network.c | 61 +-- tools/virsh-pool.c | 71 ++-- tools/virsh-snapshot.c | 60 +-- tools/virsh-volume.c | 148 ++----- tools/virsh.h | 17 + 9 files changed, 334 insertions(+), 1049 deletions(-)