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

[Osier Yang]

于 2011年06月22日 05:37, 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.

Sounds good idea, :-), will try this.

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

OK.

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

This can be both of "--live" and "--config" for an active domain,
depends on how underlying hypervisor driver implements it.

>
> or is that too long?
>

Regards
Osier