[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