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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list