[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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:

However, All of the APIs the above 7 commands use: virDomainAttachDeviceFlags,
virDomainDetachDeviceFlags, and virDomainUpdateDeviceFlags support following
3 flags:

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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]