[libvirt] [PATCH v2] virsh: Increase device-detach intelligence
Eric Blake
eblake at redhat.com
Tue Jun 14 20:06:35 UTC 2011
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 at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110614/f455ffab/attachment-0001.sig>
More information about the libvir-list
mailing list