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

Re: [libvirt] [PATCH v2] virsh: Increase device-detach intelligence



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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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