[libvirt] [PATCH] virsh: Make <mac> required when device-detaching interface
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. --- 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; + } + + 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; + +cleanup: + xmlXPathFreeContext(ctxt); + if (xml) + xmlFreeDoc(xml); + VIR_FREE(buffer); virDomainFree(dom); - return TRUE; + return ret; } -- 1.7.4
On 02/22/2011 03:16 AM, 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. --- tools/virsh.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++----------- 1 files changed, 46 insertions(+), 11 deletions(-)
I'm not sure this is patching the right place. This fixes virsh for the detach-device <interface> case, but breaks detach-device <disk> and friends (which don't have or need a <mac>). Meanwhile, this is only papering over things at the virsh, while still leaving the underlying API vulnerable (if you use python, or perl, or java, or ... bindings). Wouldn't the smarter fix be to change the underlying API, so that when detach-device is called on an <interface> and the domain only has one interface available, then that interface is detached without requiring a <mac>; therefore, the problem of requiring a <mac> should only be when there are more than one <interface> attached to the domain in the first place? That is, the detach device code that parses XML into the device that needs to be detached needs to know whether the mac address was auto-generated (good for attach, ignorable on detach with one interface, error on detach with multiple interfaces), or explicitly specified (good for attach, must match an existing an existing interface on detach). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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. 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 -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
于 2011年02月23日 12:30, Daniel Veillard 写道:
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.
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.
I guess you mean this one, :-) https://www.redhat.com/archives/libvir-list/2011-January/msg01011.html Regards Osier
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
On 02/23/2011 05:30 AM, Daniel Veillard wrote: 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.
participants (5)
-
Daniel Veillard -
Eric Blake -
Michal Privoznik -
Michal Prívozník
-
Osier Yang