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