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

Re: [libvirt] [PATCH] virsh: Make <mac> required when device-detaching interface



On 02/23/2011 05:30 AM, Daniel Veillard wrote:
On Tue, Feb 22, 2011 at 11:16:29AM +0100, Michal Privoznik wrote:
Problem is, if user does not specify mac address in input XML,
we generate a random one, which is why device-detach fails giving
a confusing error message. Therefore<mac>  needs to be required.

   Well if the domain only has one interface then if there is no<mac>
specified the semantic of the operation is still clear. Since it a very
frequent user case, I would rather not break something which was working
and has a clear semantic.
I couldn't agree more, but this is not how it works now anyway. Actually this should be the patch for https://bugzilla.redhat.com/show_bug.cgi?id=616721
But yeah, it would be better, if it works that way you've pointed out.

   IMHO we should check if the domain has multiple interface first and
only raise a problem then. I think I saw such a patch a few weeks ago
possibly in a different context though.

---
  tools/virsh.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++-----------
  1 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index 2837e0f..dfb48d2 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -8580,9 +8580,12 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)
      virDomainPtr dom;
      char *from;
      char *buffer;
-    int ret = TRUE;
+    int ret = FALSE;
      int found;
      unsigned int flags;
+    xmlDocPtr xml = NULL;
+    xmlXPathContextPtr ctxt = NULL;
+    int mac_cnt;

      if (!vshConnectionUsability(ctl, ctl->conn))
          return FALSE;
@@ -8592,14 +8595,41 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)

      from = vshCommandOptString(cmd, "file",&found);
      if (!found) {
-        virDomainFree(dom);
-        return FALSE;
+        goto cleanup;
      }

      if (virFileReadAll(from, VIRSH_MAX_XML_FILE,&buffer)<  0) {
          virshReportError(ctl);
-        virDomainFree(dom);
-        return FALSE;
+        goto cleanup;
+    }
+
+    xml = xmlReadDoc((const xmlChar *) buffer, "interface.xml", NULL,
+                     XML_PARSE_NOENT | XML_PARSE_NONET |
+                     XML_PARSE_NOWARNING);
+
+    if (!xml) {
+        vshError(ctl, "%s", _("input XML is not valid"));
+        goto cleanup;
+    }

   Hum, "valid"in XML means conformant to the DTD ot to a schemas, and
that's not something xmlReadDoc checks. The best is to say something like
   "input XML failed to parse"
i.e. it's likely to be a syntactic error, which should be reported more
precisely by libxml2

+    ctxt = xmlXPathNewContext(xml);
+    mac_cnt = virXPathNodeSet("/interface/mac", ctxt, NULL);
+
+    switch(mac_cnt) {
+        case 1:
+            break;
+
+        case 0:
+        case -1:
+            vshError(ctl, "%s", _("You must specify mac address in xml file"));
+            goto cleanup;
+            break;
+
+        default:
+            vshError(ctl, "%s", _("You must specify exactly one mac address in"
+                                  " xml file"));
+            goto cleanup;
+            break;
      }

      if (vshCommandOptBool(cmd, "persistent")) {
@@ -8610,18 +8640,23 @@ cmdDetachDevice(vshControl *ctl, const vshCmd *cmd)
      } else {
          ret = virDomainDetachDevice(dom, buffer);
      }
-    VIR_FREE(buffer);

      if (ret<  0) {
+        ret = FALSE;
          vshError(ctl, _("Failed to detach device from %s"), from);
-        virDomainFree(dom);
-        return FALSE;
-    } else {
-        vshPrint(ctl, "%s", _("Device detached successfully\n"));
+        goto cleanup;
      }

+    vshPrint(ctl, "%s", _("Device detached successfully\n"));
+    ret = TRUE;

   centralizing error handling is a good idea

+cleanup:
+    xmlXPathFreeContext(ctxt);
+    if (xml)
+        xmlFreeDoc(xml);
+    VIR_FREE(buffer);
      virDomainFree(dom);
-    return TRUE;
+    return ret;
  }



Daniel


I'll rewrite and sent v2.


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