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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list