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

[John Ferlan]

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