Re: [libvirt] [PATCH 1/2] virsh: Add options for several commands which change domain config

On 06/21/2011 03:07 AM, Osier Yang wrote: >>> This patch add two new options (--live, --current), Good. >>> and changes >>> "--persistent" into "--config", just as other similar commands, NACK. Consistency may be nice, but backwards compatibility is more important. If we were to create an aliasing mechanism, where --persistent is still recognized as an alias for --config but not explicitly documented in the help output (but still mentioned in virsh.pod), then we could use that; in fact, we could add: VSH_OFLAG_ALIAS - if set, then the help field is the primary spelling for which this option spelling is an alias {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, {"persistent", VSH_OT_BOOL, VSH_OT_ALIAS, "config"}, where only --config is documented in 'virsh help attach-device', but where 'virsh attach-device --persistent' behaves the same. But that's a much bigger prerequisite patch; the rest of your patch (adding --live and --current) is good whether or not we figure out a way to backwards-compatibly rename --persistent into --config.

>>> >>> This patch removes codes like this, leave the determination for underly s/underly/underlying/

>>> hypervisor driver. >>> --- >>> tools/virsh.c | 242 >>> ++++++++++++++++++++++++++++++++++++++++++--------------- >>> 1 files changed, 178 insertions(+), 64 deletions(-) >>> >>> diff --git a/tools/virsh.c b/tools/virsh.c >>> index abc4614..4cd2e1a 100644 >>> --- a/tools/virsh.c >>> +++ b/tools/virsh.c >>> @@ -9738,7 +9738,9 @@ static const vshCmdInfo info_attach_device[] = { >>> static const vshCmdOptDef opts_attach_device[] = { >>> {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or >>> uuid")}, >>> {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("XML file")}, >>> - {"persistent", VSH_OT_BOOL, 0, N_("persist device attachment")}, >>> + {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, >>> + {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, >>> + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, >> Maybe it's a bit early morning for me, but I didn't get this >> description. We are always affecting current domain :) How about a separate cleanup patch which unifies all of these similar strings into: "config/persistent",... N_("alter persistent configuration, effect observed on next boot, error for transient domain") "live",... N_("alter running domain, immediate effect, error for transient domain") "current",... N_("either --config or --live according to current domain state")

or is that too long?