Re: [libvirt] [PATCH v2 00/14] Use macros for more common virsh command options
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(-)