[libvirt] [PATCH v2 00/14] Use macros for more common virsh command options

[Laine Stump]

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(-)
>