Re: [PATCH 2/6] virsh: Create macro for "pool" option

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Dec 16, 2015 at 08:24:43 -0500, John Ferlan wrote:
> 
> [...]
> 
> >>  static const vshCmdOptDef opts_pool_autostart[] = {
> >> -    {.name = "pool",
> >> -     .type = VSH_OT_DATA,
> >> -     .flags = VSH_OFLAG_REQ,
> >> -     .help = N_("pool name or uuid")
> >> -    },
> >> +    OPT_POOL_COMMON

VSH_POOL_OPT_COMMON for consistency with basically all the other macros.

Also please don't include the trailing comma in the macro, it looks
weird c-wise, and will make possible regex matches for code consistency
really weird.

> >> +
> >>      {.name = "disable",
> >>       .type = VSH_OT_BOOL,
> >>       .help = N_("disable autostarting")
> > 
> > Nice. ACK
> > 
> > Should we do something similar to domain, network, ... in the rest of virsh?
> > 

Hiding stuff behind macros can be confusing sometimes, but for the most
commonly used objects it would be reasonable.

> 
> I think it would make certain things easier - I did remark on this in
> the cover letter. It gets a bit tedious to do and review; however, I
> think in the long run makes things more consistent.  There are a few
> options with "slight" differences, but nothing that cannot be worked out.

Easier? not really, it would just introduce less code when adding new
stuff at the slight cost of having to resolve the macro when looking up
what the stuff is doing. That's why only the most common objects should
be converted.

Peter

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]