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

Re: [libvirt] [PATCH 3/5] virsh-domain: Add --live, --config, --current logic to cmdDetachInterface



On 03/27/13 13:53, Martin Kletzander wrote:
On 03/21/2013 05:42 PM, Peter Krempa wrote:
Use the established approach to improve this function too.
---
  tools/virsh-domain.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
  tools/virsh.pod      | 15 +++++++++++----
  2 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 6741837..df72c78 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -9353,13 +9353,25 @@ static const vshCmdOptDef opts_detach_interface[] = {
       .help = N_("MAC address")
      },
      {.name = "persistent",
-     .type = VSH_OT_ALIAS,
-     .help = "config"
+     .type = VSH_OT_BOOL,
+     .help = N_("make live change persistent")
      },
      {.name = "config",
       .type = VSH_OT_BOOL,
       .help = N_("affect next boot")
      },
+    {.name = "live",
+     .type = VSH_OT_BOOL,
+     .help = N_("affect running domain")
+    },
+    {.name = "current",
+     .type = VSH_OT_BOOL,
+     .help = N_("affect current domain")
+    },
+    {.name = "force",
+     .type = VSH_OT_BOOL,
+     .help = N_("force device update")
+    },

Adding non-used --force paramter, copy-paste error?

      {.name = NULL}
  };

@@ -9373,12 +9385,27 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
      xmlNodePtr cur = NULL, matchNode = NULL;
      xmlBufferPtr xml_buf = NULL;
      const char *mac =NULL, *type = NULL;
-    char *doc;
+    char *doc = NULL;
      char buf[64];
      int i = 0, diff_mac;
      int ret;
      int functionReturn = false;
-    unsigned int flags;
+    unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT;
+    bool current = vshCommandOptBool(cmd, "current");
+    bool config = vshCommandOptBool(cmd, "config");
+    bool live = vshCommandOptBool(cmd, "live");
+    bool persistent = vshCommandOptBool(cmd, "persistent");
+
+    VSH_EXCLUSIVE_OPTIONS_VAR(persistent, live);

Same as in previous patch, remove this line.

+    VSH_EXCLUSIVE_OPTIONS_VAR(persistent, current);
+
+    VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
+    VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
+
+    if (config || persistent)
+        flags |= VIR_DOMAIN_AFFECT_CONFIG;
+    if (live)
+        flags |= VIR_DOMAIN_AFFECT_LIVE;

      if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
          return false;
@@ -9389,13 +9416,14 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
      if (vshCommandOptStringReq(ctl, cmd, "mac", &mac) < 0)
          goto cleanup;

-    doc = virDomainGetXMLDesc(dom, 0);
-    if (!doc)
+    if (persistent &&
+        virDomainIsActive(dom) == 1)
+        flags |= VIR_DOMAIN_AFFECT_LIVE;
+
+    if (!(doc = virDomainGetXMLDesc(dom, 0)))
          goto cleanup;

-    xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt);
-    VIR_FREE(doc);
-    if (!xml) {
+    if (!(xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt))) {
          vshError(ctl, "%s", _("Failed to get interface information"));
          goto cleanup;
      }
@@ -9460,10 +9488,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
          goto cleanup;
      }

-    if (vshCommandOptBool(cmd, "config")) {
-        flags = VIR_DOMAIN_AFFECT_CONFIG;
-        if (virDomainIsActive(dom) == 1)
-            flags |= VIR_DOMAIN_AFFECT_LIVE;
+    if (flags != 0) {
          ret = virDomainDetachDeviceFlags(dom,
                                           (char *)xmlBufferContent(xml_buf),
                                           flags);

This if seems pointless, because we usually call the *Flags() function
with flags=0, but this particular function is (for example in qemu
driver) calling the *Flags() function with AFFECT_LIVE.  The current
virsh code also adds the AFFECT_LIVE automatically, as you can see...

@@ -9479,6 +9504,7 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd)
      }

   cleanup:
+    VIR_FREE(doc);

If this fixes a leak, it could be mentioned in the commit message.

      virDomainFree(dom);
      xmlXPathFreeObject(obj);
      xmlXPathFreeContext(ctxt);
diff --git a/tools/virsh.pod b/tools/virsh.pod
index a9e8c65..ebbe201 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1876,16 +1876,23 @@ If I<--config> is specified, alter persistent configuration, effect observed
  on next boot, for compatibility purposes, I<--persistent> is alias of
  I<--config>.

-=item B<detach-interface> I<domain> I<type> [I<--mac mac>] [I<--config>]
+=item B<detach-interface> I<domain> I<type> [I<--mac mac>]
+[[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]]

  Detach a network interface from a domain.
  I<type> can be either I<network> to indicate a physical network device or
  I<bridge> to indicate a bridge to a device. It is recommended to use the
  I<mac> option to distinguish between the interfaces if more than one are
  present on the domain.
-If I<--config> is specified, alter persistent configuration, effect observed
-on next boot, for compatibility purposes, I<--persistent> is alias of
-I<--config>.
+
+If I<--live> is specified, affect a running domain.
+If I<--config> is specified, affect the next startup of a persistent domain.
+If I<--current> is specified, affect the current domain state.
+Both I<--live> and I<--config> flags may be given, but I<--current> is
+exclusive. Not specifying any flag is the same as specifying I<--current>.

I changed the last sentence to: When no flag is specified legacy API is used whose behavior depends on the hypervisor driver.

Peter


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