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

[Eric Blake]

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?

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110621/7bd7c0c5/attachment-0001.sig>