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

[Michal Privoznik]

On 21.06.2011 07:37, Osier Yang wrote:
> All of the following 7 commands just provide one option (--persistent)
> for user to specify how to affect the domain:
>     attach-device
>     detach-device
>     attach-disk
>     detach-disk
>     attach-interface
>     detach-interface
>     update-device
> 
> However, All of the APIs the above 7 commands use: virDomainAttachDeviceFlags,
> virDomainDetachDeviceFlags, and virDomainUpdateDeviceFlags support following
> 3 flags:
>     VIR_DOMAIN_AFFECT_CURRENT
>     VIR_DOMAIN_AFFECT_CONFIG
>     VIR_DOMAIN_AFFECT_LIVE
> 
> This patch add two new options (--live, --current), and changes
> "--persistent" into "--config", just as other similar commands,
I am not fully convinced about this. I mean - it would be nice to have
unified options names for these commands, but I am afraid we can't
change them.
> e.g. "schedinfo", "vcpupin".
> 
> And since the APIs are designed as: If no flag is specified, behaviour
> is different depending on hypervisor, so virsh shouldn't do things like:
> 
>     if (virDomainIsActive(dom) == 1)
>         flags |= VIR_DOMAIN_AFFECT_LIVE;
> 
> This patch removes codes like this, leave the determination for underly
> 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 :) I would append
'status' word, resulting in "affect current domain status". This makes
it clear for me. And maybe change description for 'vcpupin' command as
well. But that could be a follow up patch.

Otherwise looking good.

Michal