3:06 p.m.
On 06/06/2011 04:20 AM, Michal Prívozník wrote:
Up to now users have to give a full XML description on input when
device-detaching. If they omitted something it lead to unclear
error messages (like generated MAC wasn't found, etc.).
With this patch users can specify only those information which
specify one device sufficiently precise. Remaining information is
completed from domain.
---
diff to v1:
-rebase to current HEAD
-add a little bit comments
tools/virsh.c | 266 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
1 files changed, 250 insertions(+), 16 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index d98be1c..5c2634c 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -9302,6 +9302,226 @@ cmdAttachDevice(vshControl *ctl, const vshCmd *cmd)
return true;
}
+/**
+ * Check if n1 is superset of n2, meaning n1 contains all elements and
+ * attributes as n2 at lest. Including children.
s/lest/least/
+ * @n1 first node
+ * @n2 second node
+ * return 1 in case n1 covers n2, 0 otherwise.
You're using this as a bool, so make it bool. Return true for superset,
false for mismatch.
+ */
+static int
s/int/bool/
+vshNodeIsSuperset(xmlNodePtr n1, xmlNodePtr n2) {
+ xmlNodePtr child1, child2;
+ xmlAttrPtr attr1, attr2;
+ int found;
Looks like you are also using found as a bool.
+
+ if (!n1 && !n2)
+ return 1;
s/1/true/
+
+ if (!n1 || !n2)
+ return 0;
s/0/false/, etc.
+
+ if (!xmlStrEqual(n1->name, n2->name))
+ return 0;
+
+ /* Iterate over n2 attributes and check if n1 contains them*/
style: space before comment end: "them */"
+ attr2 = n2->properties;
+ while (attr2) {
+ if (attr2->type == XML_ATTRIBUTE_NODE) {
+ attr1 = n1->properties;
+ found = 0;
+ while (attr1) {
+ if (xmlStrEqual(attr1->name, attr2->name)) {
+ found = 1;
+ break;
+ }
+ attr1 = attr1->next;
+ }
+ if (!found)
+ return 0;
+ if (!xmlStrEqual(BAD_CAST virXMLPropString(n1, (const char *)
attr1->name),
+ BAD_CAST virXMLPropString(n2, (const char *)
attr2->name)))
Why xmlStrEqual and the nasty casting? Why not just STREQ(), since
virXMLPropString is already char*?
+ return 0;
+ }
+ attr2 = attr2->next;
+ }
+
+ /* and now iterate over n2 children */
+ child2 = n2->children;
+ while (child2) {
+ if (child2->type == XML_ELEMENT_NODE) {
+ child1 = n1->children;
+ found = 0;
+ while (child1) {
+ if (child1->type == XML_ELEMENT_NODE &&
+ xmlStrEqual(child1->name, child2->name)) {
+ found = 1;
+ break;
+ }
This probably won't work for XML that can be <oneOrMore> in the rng
schema, especially if you also have <interleave> in the mix. For
example, these two should be treated as equivalent (both serve as a
superset of the other), but your algorithm will compare n2->b[0] with
n1->b[0], followed by n2->b[1] with n1->b[0]:
<a>
<b>one</b>
<b>two</b>
</a>
<a>
<b>two</b>
<b>one</b>
</a>
Thankfully, I don't think we are really using any <interleave> or
<oneOrMore> subelements in any of the xml elements you plan on comparing
via this function, but what happens when that changes?
+ child1 = child1->next;
+ }
+ if (!found)
+ return 0;
+ if (!vshNodeIsSuperset(child1, child2))
+ return 0;
+ }
+ child2 = child2->next;
+ }
+
+ return 1;
+}
+
+/**
+ * To given domain and (probably incomplete) device XML specification try to
s/To given/For a given/
> + * find such device in domain and complete missing parts. This is however
> + * possible when given device XML is sufficiently precise so it addresses only
> + * one device.
> + * @ctl vshControl for error messages printing
> + * @dom domain
> + * @oldXML device XML before
> + * @newXML and after completion
> + * Returns -2 when no such device exists in domain, -3 when given XML selects many
> + * (is too ambiguous), 0 in case of success. Otherwise returns -1. @newXML
> + * is touched only in case of success.
+ */
+static int
> +vshCompleteXMLFromDomain(vshControl *ctl, virDomainPtr dom, char
*oldXML,
> + char **newXML) {
> + int funcRet = -1;
> + char *domXML = NULL;
> + xmlDocPtr domDoc = NULL, devDoc = NULL;
> + xmlNodePtr node = NULL;
> + xmlXPathContextPtr domCtxt = NULL, devCtxt = NULL;
> + xmlNodePtr *devices = NULL;
> + xmlSaveCtxtPtr sctxt = NULL;
> + int devices_size;
> + char *xpath;
> + xmlBufferPtr buf = NULL;
> +
> + if (!(domXML = virDomainGetXMLDesc(dom, 0))) {
> + vshError(ctl, _("couldn't get XML description of domain %s"),
> + virDomainGetName(dom));
> + goto cleanup;
> + }
> +
> + if (!(domDoc = xmlReadDoc(BAD_CAST domXML, "domain.xml", NULL,
> + XML_PARSE_NOENT | XML_PARSE_NONET |
> + XML_PARSE_NOERROR | XML_PARSE_NOWARNING))) {
Commit b9e51e56 recently changed a lot of xmlReadDoc to the simpler
virXMLParseString; this should do likewise.
+ /* and refine */
+ int i = 0;
+ while (i < devices_size) {
+ if (!vshNodeIsSuperset(devices[i], node)) {
+ if (devices_size == 1) {
+ VIR_FREE(devices);
+ devices_size = 0;
+ } else {
+ memmove(devices + i, devices + i + 1,
+ sizeof(*devices) * (devices_size-i-1));
Is this memmove necessary (it scales quadratically)? Or would it be
better to leave the devices array untouched, and instead initialize 'int
index = -1' before the loop, and on match if it is still -1 then set it
to the match index otherwise error out with -3 (ambiguous), and after
the loop error out with -2 (missing) if it is still -1 (this would scale
linearly).
+ if (ret < 0) {
+ if (ret == -2) {
+ vshError(ctl, _("no such device in %s"), virDomainGetName(dom));
+ } else if (ret == -3) {
+ vshError(ctl, "%s", _("given XML selects too many devices.
"
+ "Please, be more specific"));
+ } else {
+ /* vshCompleteXMLFromDomain() already printed error message,
+ * so nothing to do here. */
That's awkward. Generally, a helper function should either print no
messages, or print all possible messages. But I guess it works here
because of the distinct return values, even though it is a bit harder to
audit the division of labor.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org