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

Osier Yang jyang at redhat.com
Tue Feb 14 04:56:21 UTC 2012


On 02/14/2012 05:59 AM, Eric Blake wrote:
> 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.

That was the intention, the function could be used to find disk
in other special way in future. Such as find a disk with
device == lun.

>
>> +{
>> +    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).

Agreed, virXMLParseString() is better. :)

>
>> +                     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?

Oops, yes.

>
>> +                    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)?

It should be enum, will update.

>
>> +
>> +/* 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;

Agreed.

>
>> +
>> +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

Agreed, nicer way.

Osier





More information about the libvir-list mailing list