On 06/21/2011 04:55 PM, Michal Privoznik wrote:
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.
That's what I'm worried about too, changing "--persistent" into
"--config" may affect already existed scripts based on virsh. But
considering the unification, it might be better to change it.
Or perhaps we can just leave "--persistent" unchanged, but it
might be quite confused for user.
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.
These descriptions used widely in the codes, agree it's a bit confused.
Otherwise looking good.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list