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

Re: [libvirt] [PATCH 1/4] virsh: Two new helper functions for disk device changes



On 02/03/2012 02:23 AM, Osier Yang wrote:
> vshFindDisk is to find the disk node in xml doc with given target
> and flags (indicates disk type, normal disk or changeable disk).
> 
> vshPrepareDiskXML is to make changes on the disk node (e.g. create
> and insert the new <source> node for inserting media of CDROM drive).
> ---
>  tools/virsh.c |  207 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 207 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index af78102..ca69f30 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -14210,6 +14210,213 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd)
>      return functionReturn;
>  }
>  
> +typedef enum {
> +    VSH_FIND_DISK_NORMAL,
> +    VSH_FIND_DISK_CHANGEABLE,
> +} vshFindDiskFlags;

If these were really flags, they would be (1 << 0) and (1 << 1), not 0
and 1.

> +
> +
> +/* Helper function to find disk device in XML doc.  Returns the disk
> + * node on success, or NULL on failure. Caller must free the result
> + * @type can be either VSH_FIND_DISK_NORMAL or VSH_FIND_DISK_CHANGEABLE.
> + */
> +static xmlNodePtr
> +vshFindDisk(const char *doc,
> +            const char *target,
> +            unsigned int flags)

I think it may be simpler to just treat this parameter as bool
changeable; and pass false for normal disks and true for cdrom, unless
you really do plan to add more flags later.

> +{
> +    xmlDocPtr xml = NULL;
> +    xmlXPathObjectPtr obj= NULL;
> +    xmlXPathContextPtr ctxt = NULL;
> +    xmlNodePtr cur = NULL;
> +    xmlNodePtr ret = NULL;
> +    int i = 0;
> +
> +    xml = xmlReadDoc((const xmlChar *) doc, "domain.xml", NULL,

Shouldn't we be using something like virXMLParseString(), and not raw
xmlReadDoc(), for better error reporting purposes?  No one else in this
file uses xmlReadDoc, and we _aren't_ reading from a file named
'domain.xml', but from an in-memory string (where using something like
_("(domain_definition)") will give a better error message on a parse error).

> +                     XML_PARSE_NOENT | XML_PARSE_NONET |
> +                     XML_PARSE_NOWARNING);
> +
> +    if (!xml) {
> +        vshError(NULL, "%s", _("Failed to get disk information"));
> +        goto cleanup;
> +    }
> +
> +    ctxt = xmlXPathNewContext(xml);
> +    if (!ctxt) {
> +        vshError(NULL, "%s", _("Failed to get disk information"));
> +        goto cleanup;
> +    }
> +
> +    obj = xmlXPathEval(BAD_CAST "/domain/devices/disk", ctxt);
> +    if ((obj == NULL) ||
> +        (obj->type != XPATH_NODESET) ||
> +        (obj->nodesetval == NULL) ||
> +        (obj->nodesetval->nodeNr == 0)) {
> +        vshError(NULL, "%s", _("Failed to get disk information"));
> +        goto cleanup;
> +    }
> +
> +    /* search target */
> +    for (; i < obj->nodesetval->nodeNr; i++) {
> +        bool is_supported = true;
> +
> +        if (flags & VSH_FIND_DISK_CHANGEABLE) {
> +            xmlNodePtr n = obj->nodesetval->nodeTab[i];
> +            is_supported = false;
> +
> +            /* Check if the disk is CDROM or floppy disk */
> +            if (xmlStrEqual(n->name, BAD_CAST "disk")) {
> +                char *device_value = virXMLPropString(n, "device");
> +
> +                if (STREQ(device_value, "cdrom") ||
> +                    STREQ(device_value, "floppy"))
> +                    is_supported = true;
> +
> +                VIR_FREE(device_value);
> +            }
> +        }

Right here, couldn't you just 'continue' the outer loop if is_supported
is false, rather than spinning your wheels...

> +
> +        cur = obj->nodesetval->nodeTab[i]->children;
> +        while (cur != NULL) {
> +            if (cur->type == XML_ELEMENT_NODE &&
> +                xmlStrEqual(cur->name, BAD_CAST "target")) {
> +                char *tmp = virXMLPropString(cur, "dev");
> +
> +                if (is_supported &&
> +                    STREQ_NULLABLE(tmp, target)) {

...on an inner loop that will never resolve?

> +                    ret = xmlCopyNode(obj->nodesetval->nodeTab[i], 1);
> +                    VIR_FREE(tmp);
> +                    goto cleanup;
> +                }
> +                VIR_FREE(tmp);
> +            }
> +            cur = cur->next;
> +        }
> +    }
> +
> +    vshError(NULL, _("No found disk whose target is %s"), target);
> +
> +cleanup:
> +    xmlXPathFreeObject(obj);
> +    xmlXPathFreeContext(ctxt);
> +    xmlFreeDoc(xml);
> +    return ret;
> +}
> +
> +typedef enum {
> +    VSH_PREPARE_DISK_XML_NONE = 0,
> +    VSH_PREPARE_DISK_XML_EJECT = (1 << 0),
> +    VSH_PREPARE_DISK_XML_INSERT = (1 << 1),
> +    VSH_PREPARE_DISK_XML_UPDATE = (1 << 2),
> +} vshPrepareDiskXMLFlags;

Can you really eject and update at the same time?  That is, should this
be a linear enum (1, 2, 3) rather than bits (1, 2, 4)?

> +
> +/* Helper function to prepare disk XML. Could be used for disk
> + * detaching, media changing(ejecting, inserting, updating)
> + * for changedable disk. Returns the processed XML as string on

s/changedable/changeable/

> + * success, or NULL on failure. Caller must free the result.
> + */
> +static char *
> +vshPrepareDiskXML(xmlNodePtr disk_node,
> +                  const char *source,
> +                  const char *target,
> +                  unsigned int flags)
> +{
> +    xmlNodePtr cur = NULL;
> +    xmlBufferPtr xml_buf = NULL;
> +    const char *disk_type = NULL;
> +    const char *device_type = NULL;
> +    xmlNodePtr new_node = NULL;
> +    char *ret = NULL;
> +
> +    if (!disk_node)
> +        return NULL;
> +
> +    xml_buf = xmlBufferCreate();
> +    if (!xml_buf) {
> +        vshError(NULL, "%s", _("Failed to allocate memory"));
> +        return NULL;
> +    }
> +
> +    device_type = virXMLPropString(disk_node, "device");
> +

> +
> +    if (xmlNodeDump(xml_buf, NULL, disk_node, 0, 0) < 0) {
> +        vshError(NULL, "%s", _("Failed to create XML"));
> +        goto error;
> +    }
> +
> +    goto cleanup;
> +
> +error:
> +    xmlBufferFree(xml_buf);
> +    xml_buf = NULL;

I've generally seen the style:

    ret = ...
cleanup:
    ...
    return ret;
error:
    ...
    goto cleanup:

more than your style of:

    ret = ...
    goto cleanup;
error:
    ...
cleanup:
    ...
    return ret;

> +
> +cleanup:
> +    VIR_FREE(device_type);
> +    VIR_FREE(disk_type);
> +    if (xml_buf) {
> +        ret = vshCalloc(NULL, xmlBufferLength(xml_buf), sizeof(char));

sizeof(char) is always 1; it always looks fishy to me to see it without
good reason.  vshCalloc generally wants a non-NULL first parameter, for
better logging of OOM messages.  Is it any easier to just use:

if (VIR_ALLOC_N(ret, xmlBufferLength(xml_buf)) < 0)
    error reporting

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
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]