On 01/06/2016 01:58 PM, Laine Stump wrote:
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).
Understood; however, at least they're more or less easily found especially if you use cscope. There are certainly some macros in the source code which are far more obfuscated!
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. :-)
yes, painfully ;-)
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)
Newsflash - twitter is apparently expanding to allow 10000 characters. Maybe now I'll be able to start tweeting (yeah, right, not!)
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.
I was following Peter's suggested naming: http://www.redhat.com/archives/libvir-list/2015-December/msg00675.html but I have no favorite... If others chime in and agree, then I'm fine with switching. thanks for the comments - John
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(-)