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(-) >> > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
