[libvirt] [PATCH 0/3 v4] New command for changing media

[PATCH 0/3] introduces two new helper functions: vshFindDisk is to find the disk XML node in xml doc with the type (indicates it's normal disk or changeable disk). VshPrepareDiskXML is to prepare the disk XML for disk changing commands' use, such as detach-disk. The purpose of this patch is to prepare for upcoming new command "change-disk", which will use the two helper functions too. Please see [PATCH 3/3] for a overall description of the new command. v2 - v3 * Update vshFindDisk to support search the disk node with both the source path and target name. * Change names of the two new enums ([PATCH 1/3]). * Use virXMLParseStringCtxt instead of xmlReadDoc * Use VIR_ALLOC_N instead of vshCalloc. * [PATCH 3/4] and [PATCH 4/4] are merged into [PATCH 3/3] * Change option "--target" into "--path". * Defaults to use "--update" if none of "--eject", "--insert", and "--update" is specified. * Update docs to be consistent with the changes on codes. * Other nits. v3 - v4: * No changes. Osier Yang (3) virsh: Two new helper functions for disk device changes virsh: Use vshFindDisk and vshPrepareDiskXML in virsh: New command cmdChangeMedia tools/virsh.c | 418 ++++++++++++++++++++++++++++++++++++++++++++++++------- tools/virsh.pod | 33 ++++- 2 files changed, 395 insertions(+), 56 deletions(-) Regards, Osier

vshFindDisk is to find the disk node in xml doc with given source path or target of disk device, and type (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). They are marked as unused temporarily. --- tools/virsh.c | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 215 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 66bbb0c..b65114f 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -14321,6 +14321,221 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) return functionReturn; } +typedef enum { + VSH_FIND_DISK_NORMAL, + VSH_FIND_DISK_CHANGEABLE, +} vshFindDiskType; + +/* Helper function to find disk device in XML doc. Returns the disk + * node on success, or NULL on failure. Caller must free the result + * @path: Fully-qualified path or target of disk device. + * @type: Either VSH_FIND_DISK_NORMAL or VSH_FIND_DISK_CHANGEABLE. + */ +ATTRIBUTE_UNUSED +static xmlNodePtr +vshFindDisk(const char *doc, + const char *path, + int type) +{ + xmlDocPtr xml = NULL; + xmlXPathObjectPtr obj= NULL; + xmlXPathContextPtr ctxt = NULL; + xmlNodePtr cur = NULL; + xmlNodePtr ret = NULL; + int i = 0; + + xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt); + if (!xml) { + 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 disk using @path */ + for (; i < obj->nodesetval->nodeNr; i++) { + bool is_supported = true; + + if (type == 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); + } + + if (!is_supported) + continue; + } + + cur = obj->nodesetval->nodeTab[i]->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE) { + char *tmp = NULL; + + if (xmlStrEqual(cur->name, BAD_CAST "source")) { + if ((tmp = virXMLPropString(cur, "file")) || + (tmp = virXMLPropString(cur, "dev")) || + (tmp = virXMLPropString(cur, "dir")) || + (tmp = virXMLPropString(cur, "name"))) { + } + } else if (xmlStrEqual(cur->name, BAD_CAST "target")) { + tmp = virXMLPropString(cur, "dev"); + } + + if (STREQ_NULLABLE(tmp, path)) { + ret = xmlCopyNode(obj->nodesetval->nodeTab[i], 1); + VIR_FREE(tmp); + goto cleanup; + } + VIR_FREE(tmp); + } + cur = cur->next; + } + } + + vshError(NULL, _("No found disk whose source path or target is %s"), path); + +cleanup: + xmlXPathFreeObject(obj); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xml); + return ret; +} + +typedef enum { + VSH_PREPARE_DISK_XML_NONE = 0, + VSH_PREPARE_DISK_XML_EJECT, + VSH_PREPARE_DISK_XML_INSERT, + VSH_PREPARE_DISK_XML_UPDATE, +} vshPrepareDiskXMLType; + +/* Helper function to prepare disk XML. Could be used for disk + * detaching, media changing(ejecting, inserting, updating) + * for changeable disk. Returns the processed XML as string on + * success, or NULL on failure. Caller must free the result. + */ +ATTRIBUTE_UNUSED +static char * +vshPrepareDiskXML(xmlNodePtr disk_node, + const char *source, + const char *path, + int type) +{ + 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 (STREQ_NULLABLE(device_type, "cdrom") || + STREQ_NULLABLE(device_type, "floppy")) { + bool has_source = false; + disk_type = virXMLPropString(disk_node, "type"); + + cur = disk_node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE && + xmlStrEqual(cur->name, BAD_CAST "source")) { + has_source = true; + break; + } + cur = cur->next; + } + + if (!has_source) { + if (type == VSH_PREPARE_DISK_XML_EJECT) { + vshError(NULL, _("The disk device whose source path or target " + "is '%s' doesn't have media"), path); + goto error; + } + + if (source) { + new_node = xmlNewNode(NULL, BAD_CAST "source"); + xmlNewProp(new_node, (const xmlChar *)disk_type, + (const xmlChar *)source); + xmlAddChild(disk_node, new_node); + } else if (type == VSH_PREPARE_DISK_XML_INSERT) { + vshError(NULL, _("No source is specified for inserting media")); + goto error; + } else if (type == VSH_PREPARE_DISK_XML_UPDATE) { + vshError(NULL, _("No source is specified for updating media")); + goto error; + } + } + + if (has_source) { + if (type == VSH_PREPARE_DISK_XML_INSERT) { + vshError(NULL, _("The disk device whose source path or target " + "is '%s' already has media"), path); + goto error; + } + + /* Remove the source if it tends to eject/update media. */ + xmlUnlinkNode(cur); + xmlFreeNode(cur); + + if (source && (type == VSH_PREPARE_DISK_XML_UPDATE)) { + new_node = xmlNewNode(NULL, BAD_CAST "source"); + xmlNewProp(new_node, (const xmlChar *)disk_type, + (const xmlChar *)source); + xmlAddChild(disk_node, new_node); + } + } + } + + if (xmlNodeDump(xml_buf, NULL, disk_node, 0, 0) < 0) { + vshError(NULL, "%s", _("Failed to create XML")); + goto error; + } + + goto cleanup; + +cleanup: + VIR_FREE(device_type); + VIR_FREE(disk_type); + if (xml_buf) { + if (VIR_ALLOC_N(ret, xmlBufferLength(xml_buf)) < 0) { + virReportOOMError(); + return NULL; + } + memcpy(ret, (char *)xmlBufferContent(xml_buf), xmlBufferLength(xml_buf)); + xmlBufferFree(xml_buf); + } + return ret; + +error: + xmlBufferFree(xml_buf); + xml_buf = NULL; + goto cleanup; +} + /* * "detach-disk" command */ -- 1.7.7.3

On 02/27/2012 05:11 AM, Osier Yang wrote:
vshFindDisk is to find the disk node in xml doc with given source path or target of disk device, and type (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).
They are marked as unused temporarily. --- tools/virsh.c | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 215 insertions(+), 0 deletions(-)
+ if (STREQ_NULLABLE(device_type, "cdrom") || + STREQ_NULLABLE(device_type, "floppy")) { + bool has_source = false; + disk_type = virXMLPropString(disk_node, "type"); + + cur = disk_node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE && + xmlStrEqual(cur->name, BAD_CAST "source")) { + has_source = true; + break; + } + cur = cur->next; + } + + if (!has_source) { + if (type == VSH_PREPARE_DISK_XML_EJECT) { + vshError(NULL, _("The disk device whose source path or target " + "is '%s' doesn't have media"), path);
If there is no source, then the disk device was found via target, not source path :) Avoid the problem, as well as making things shorter, by changing the error message to: "The disk device '%s' doesn't have media"
+ + if (has_source) { + if (type == VSH_PREPARE_DISK_XML_INSERT) { + vshError(NULL, _("The disk device whose source path or target " + "is '%s' already has media"), path);
Along those lines, a consistent error message would be: "The disk device '%s' already has media" ACK with those nits fixed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

The first use of the two new helper functions. --- tools/virsh.c | 65 ++++++++------------------------------------------------ 1 files changed, 10 insertions(+), 55 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index b65114f..eeb2426 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -14331,7 +14331,6 @@ typedef enum { * @path: Fully-qualified path or target of disk device. * @type: Either VSH_FIND_DISK_NORMAL or VSH_FIND_DISK_CHANGEABLE. */ -ATTRIBUTE_UNUSED static xmlNodePtr vshFindDisk(const char *doc, const char *path, @@ -14429,7 +14428,6 @@ typedef enum { * for changeable disk. Returns the processed XML as string on * success, or NULL on failure. Caller must free the result. */ -ATTRIBUTE_UNUSED static char * vshPrepareDiskXML(xmlNodePtr disk_node, const char *source, @@ -14555,18 +14553,14 @@ static const vshCmdOptDef opts_detach_disk[] = { static bool cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) { - xmlDocPtr xml = NULL; - xmlXPathObjectPtr obj=NULL; - xmlXPathContextPtr ctxt = NULL; - xmlNodePtr cur = NULL; - xmlBufferPtr xml_buf = NULL; + char *disk_xml = NULL; virDomainPtr dom = NULL; const char *target = NULL; char *doc; - int i = 0, diff_tgt; int ret; bool functionReturn = false; unsigned int flags; + xmlNodePtr disk_node = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -14581,60 +14575,22 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) if (!doc) goto cleanup; - xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt); - VIR_FREE(doc); - if (!xml) { - vshError(ctl, "%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(ctl, "%s", _("Failed to get disk information")); - goto cleanup; - } - - /* search target */ - for (; i < obj->nodesetval->nodeNr; i++) { - cur = obj->nodesetval->nodeTab[i]->children; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE && - xmlStrEqual(cur->name, BAD_CAST "target")) { - char *tmp_tgt = virXMLPropString(cur, "dev"); - diff_tgt = STREQ(tmp_tgt, target); - VIR_FREE(tmp_tgt); - if (diff_tgt) { - goto hit; - } - } - cur = cur->next; - } - } - vshError(ctl, _("No found disk whose target is %s"), target); - goto cleanup; - - hit: - xml_buf = xmlBufferCreate(); - if (!xml_buf) { - vshError(ctl, "%s", _("Failed to allocate memory")); + if (!(disk_node = vshFindDisk(doc, target, VSH_FIND_DISK_NORMAL))) goto cleanup; - } - if (xmlNodeDump(xml_buf, xml, obj->nodesetval->nodeTab[i], 0, 0) < 0) { - vshError(ctl, "%s", _("Failed to create XML")); + if (!(disk_xml = vshPrepareDiskXML(disk_node, NULL, NULL, + VSH_PREPARE_DISK_XML_NONE))) goto cleanup; - } if (vshCommandOptBool(cmd, "persistent")) { flags = VIR_DOMAIN_AFFECT_CONFIG; if (virDomainIsActive(dom) == 1) flags |= VIR_DOMAIN_AFFECT_LIVE; ret = virDomainDetachDeviceFlags(dom, - (char *)xmlBufferContent(xml_buf), + disk_xml, flags); } else { - ret = virDomainDetachDevice(dom, (char *)xmlBufferContent(xml_buf)); + ret = virDomainDetachDevice(dom, disk_xml); } if (ret != 0) { @@ -14645,10 +14601,9 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) } cleanup: - xmlXPathFreeObject(obj); - xmlXPathFreeContext(ctxt); - xmlFreeDoc(xml); - xmlBufferFree(xml_buf); + xmlFreeNode(disk_node); + VIR_FREE(disk_xml); + VIR_FREE(doc); if (dom) virDomainFree(dom); return functionReturn; -- 1.7.7.3

On 02/27/2012 05:11 AM, Osier Yang wrote:
The first use of the two new helper functions. --- tools/virsh.c | 65 ++++++++------------------------------------------------ 1 files changed, 10 insertions(+), 55 deletions(-)
ACK, and nice cleanup. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

One could use it to eject, insert, or update media of the CDROM or floppy drive. See the documents for more details. --- tools/virsh.c | 142 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 33 ++++++++++++- 2 files changed, 172 insertions(+), 3 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index eeb2426..11aa042 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -14610,6 +14610,147 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) } /* + * "change-media" command + */ +static const vshCmdInfo info_change_media[] = { + {"help", N_("Change media of CD or floppy drive")}, + {"desc", N_("Change media of CD or floppy drive.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_change_media[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"path", VSH_OT_DATA, VSH_OFLAG_REQ, N_("Fully-qualified path or " + "target of disk device")}, + {"source", VSH_OT_DATA, 0, N_("source of the media")}, + {"eject", VSH_OT_BOOL, 0, N_("Eject the media")}, + {"insert", VSH_OT_BOOL, 0, N_("Insert the media")}, + {"update", VSH_OT_BOOL, 0, N_("Update the media")}, + {"current", VSH_OT_BOOL, 0, N_("can be either or both of --live and --config, " + "depends on implementation of hypervisor driver")}, + {"live", VSH_OT_BOOL, 0, N_("alter live configuration of running domain")}, + {"config", VSH_OT_BOOL, 0, N_("alter persistent configuration, effect observed on next boot")}, + {"force", VSH_OT_BOOL, 0, N_("force media insertion")}, + {NULL, 0, 0, NULL} +}; + +static bool +cmdChangeMedia(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + const char *source = NULL; + const char *path = NULL; + const char *doc = NULL; + xmlNodePtr disk_node = NULL; + const char *disk_xml = NULL; + int flags = 0; + int config, live, current, force = 0; + int eject, insert, update = 0; + bool ret = false; + int prepare_type = 0; + const char *action = NULL; + + config = vshCommandOptBool(cmd, "config"); + live = vshCommandOptBool(cmd, "live"); + current = vshCommandOptBool(cmd, "current"); + force = vshCommandOptBool(cmd, "force"); + eject = vshCommandOptBool(cmd, "eject"); + insert = vshCommandOptBool(cmd, "insert"); + update = vshCommandOptBool(cmd, "update"); + + if ((eject && insert) || + (insert && update) || + (eject && update)) { + vshError(ctl, "%s", _("--eject, --insert, and --update must be specified " + "exclusively.")); + return false; + } + + if (eject) { + prepare_type = VSH_PREPARE_DISK_XML_EJECT; + action = "eject"; + } + + if (insert) { + prepare_type = VSH_PREPARE_DISK_XML_INSERT; + action = "insert"; + } + + if (update) { + prepare_type = VSH_PREPARE_DISK_XML_UPDATE; + action = "update"; + } + + /* Defaults to "update" */ + if (!eject && !insert && !update) { + prepare_type = VSH_PREPARE_DISK_XML_UPDATE; + action = "update"; + } + + if (current) { + if (live || config) { + vshError(ctl, "%s", _("--current must be specified exclusively")); + return false; + } + flags = VIR_DOMAIN_AFFECT_CURRENT; + } else { + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + } + + if (force) + flags |= VIR_DOMAIN_DEVICE_MODIFY_FORCE; + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + goto cleanup; + + if (vshCommandOptString(cmd, "path", &path) <= 0) + goto cleanup; + + if (vshCommandOptString(cmd, "source", &source) < 0) + goto cleanup; + + if (insert && !source) { + vshError(ctl, "%s", _("No disk source specified for inserting")); + goto cleanup; + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) + doc = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE); + else + doc = virDomainGetXMLDesc(dom, 0); + if (!doc) + goto cleanup; + + if (!(disk_node = vshFindDisk(doc, path, VSH_FIND_DISK_CHANGEABLE))) + goto cleanup; + + if (!(disk_xml = vshPrepareDiskXML(disk_node, source, path, prepare_type))) + goto cleanup; + + if (virDomainUpdateDeviceFlags(dom, disk_xml, flags) != 0) { + vshError(ctl, _("Failed to %s media"), action); + goto cleanup; + } + + vshPrint(ctl, _("succeeded to %s media\n"), action); + ret = true; + +cleanup: + VIR_FREE(doc); + xmlFreeNode(disk_node); + VIR_FREE(disk_xml); + if (dom) + virDomainFree(dom); + return ret; +} + +/* * "cpu-compare" command */ static const vshCmdInfo info_cpu_compare[] = { @@ -16705,6 +16846,7 @@ static const vshCmdDef domManagementCmds[] = { #ifndef WIN32 {"console", cmdConsole, opts_console, info_console, 0}, #endif + {"change-media", cmdChangeMedia, opts_change_media, info_change_media, 0}, {"cpu-baseline", cmdCPUBaseline, opts_cpu_baseline, info_cpu_baseline, 0}, {"cpu-compare", cmdCPUCompare, opts_cpu_compare, info_cpu_compare, 0}, {"create", cmdCreate, opts_create, info_create, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index 2956c81..162d046 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1490,9 +1490,10 @@ from the domain. =item B<detach-interface> I<domain-id> I<type> [I<--mac mac>] Detach a network interface from a domain. -I<type> can be either I<network> to indicate a physical network device or I<bridge> to indicate a bridge to a device. -It is recommended to use the I<mac> option to distinguish between the interfaces -if more than one are present on the domain. +I<type> can be either I<network> to indicate a physical network device or +I<bridge> to indicate a bridge to a device. It is recommended to use the +I<mac> option to distinguish between the interfaces if more than one are +present on the domain. =item B<update-device> I<domain-id> I<file> [I<--persistent>] [I<--force>] @@ -1503,6 +1504,32 @@ option can be used to force device update, e.g., to eject a CD-ROM even if it is locked/mounted in the domain. See the documentation to learn about libvirt XML format for a device. +=item B<change-media> I<domain-id> I<path> [I<--eject>] [I<--insert>] +[I<--update>] [I<source>] [I<--force>] [I<--current>] [I<--live>] [I<--config>] + +Change media of CDROM or floppy drive. I<path> can be the fully-qualified path +or the unique target name (<target dev='hdc'>) of the disk device. I<source> +specifies the path of the media to be inserted or updated. + +I<--eject> indicates the media will be ejected. +I<--insert> indicates the media will be inserted. I<source> must be specified. +If the device has source (e.g. <source file='media'>), and I<source> is not +specified, I<--update> is equal to I<--eject>. If the device has no source, +and I<source> is specified, I<--update> is equal to I<--insert>. If the device +has source, and I<source> is specified, I<--update> behaves like combination +of I<--eject> and I<--insert>. +If none of I<--eject>, I<--insert>, and I<--update> is specified, I<--update> +is used by default. +The I<--force> option can be used to force media changing. +If I<--live> is specified, alter live configuration of running guest. +If I<--config> is specified, alter persistent configuration, effect observed +on next boot. +I<--current> can be either or both of I<live> and I<config>, depends on +the hypervisor's implementation. +Both I<--live> and I<--config> flags may be given, but I<--current> is +exclusive. If no flag is specified, behavior is different depending +on hypervisor. + =back =head1 NODEDEV COMMANDS -- 1.7.7.3

On 02/27/2012 05:11 AM, Osier Yang wrote:
One could use it to eject, insert, or update media of the CDROM or floppy drive. See the documents for more details.
s/documents/documentation/
--- tools/virsh.c | 142 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 33 ++++++++++++- 2 files changed, 172 insertions(+), 3 deletions(-)
+static const vshCmdOptDef opts_change_media[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"path", VSH_OT_DATA, VSH_OFLAG_REQ, N_("Fully-qualified path or " + "target of disk device")}, + {"source", VSH_OT_DATA, 0, N_("source of the media")}, + {"eject", VSH_OT_BOOL, 0, N_("Eject the media")}, + {"insert", VSH_OT_BOOL, 0, N_("Insert the media")}, + {"update", VSH_OT_BOOL, 0, N_("Update the media")},
I still wonder if --mode={eject|insert|update} would have been any easier to code, but that's just painting the bikeshed, so don't worry about it.
+static bool +cmdChangeMedia(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + const char *source = NULL; + const char *path = NULL; + const char *doc = NULL; + xmlNodePtr disk_node = NULL; + const char *disk_xml = NULL; + int flags = 0; + int config, live, current, force = 0;
s/int/bool/
+ int eject, insert, update = 0;
s/int/bool/
+ bool ret = false; + int prepare_type = 0; + const char *action = NULL; + + config = vshCommandOptBool(cmd, "config"); + live = vshCommandOptBool(cmd, "live"); + current = vshCommandOptBool(cmd, "current"); + force = vshCommandOptBool(cmd, "force"); + eject = vshCommandOptBool(cmd, "eject"); + insert = vshCommandOptBool(cmd, "insert"); + update = vshCommandOptBool(cmd, "update");
Particularly since you are assigning all 7 variables from vshCommandOptBool.
+ + if ((eject && insert) || + (insert && update) || + (eject && update)) {
A shorter way to write this: if (eject + insert + update > 1) {
+ vshError(ctl, "%s", _("--eject, --insert, and --update must be specified " + "exclusively.")); + return false; + } + + if (eject) { + prepare_type = VSH_PREPARE_DISK_XML_EJECT; + action = "eject"; + } + + if (insert) { + prepare_type = VSH_PREPARE_DISK_XML_INSERT; + action = "insert"; + } + + if (update) { + prepare_type = VSH_PREPARE_DISK_XML_UPDATE; + action = "update"; + } + + /* Defaults to "update" */ + if (!eject && !insert && !update) { + prepare_type = VSH_PREPARE_DISK_XML_UPDATE; + action = "update";
You can combine these two clauses: if (update || (!eject && !insert)) {
+ + if (virDomainUpdateDeviceFlags(dom, disk_xml, flags) != 0) { + vshError(ctl, _("Failed to %s media"), action);
Translators won't like this; there are languages where the spelling of the rest of the sentence depends on the gender of the word in 'action'. Better would be: "Failed to complete '%s' action on media"
+ goto cleanup; + } + + vshPrint(ctl, _("succeeded to %s media\n"), action);
and again, better would be: "Successfully completed '%s' action on media'
@@ -16705,6 +16846,7 @@ static const vshCmdDef domManagementCmds[] = { #ifndef WIN32 {"console", cmdConsole, opts_console, info_console, 0}, #endif + {"change-media", cmdChangeMedia, opts_change_media, info_change_media, 0},
Sorting - change-media comes _before_ console
+++ b/tools/virsh.pod @@ -1490,9 +1490,10 @@ from the domain. =item B<detach-interface> I<domain-id> I<type> [I<--mac mac>]
Detach a network interface from a domain. -I<type> can be either I<network> to indicate a physical network device or I<bridge> to indicate a bridge to a device. -It is recommended to use the I<mac> option to distinguish between the interfaces -if more than one are present on the domain. +I<type> can be either I<network> to indicate a physical network device or +I<bridge> to indicate a bridge to a device. It is recommended to use the +I<mac> option to distinguish between the interfaces if more than one are +present on the domain.
Unrelated hunk; for backport purposes, it would be nicer if you split this cleanup hunk into a separate (pre-approved) patch.
=item B<update-device> I<domain-id> I<file> [I<--persistent>] [I<--force>]
@@ -1503,6 +1504,32 @@ option can be used to force device update, e.g., to eject a CD-ROM even if it is locked/mounted in the domain. See the documentation to learn about libvirt XML format for a device.
+=item B<change-media> I<domain-id> I<path> [I<--eject>] [I<--insert>] +[I<--update>] [I<source>] [I<--force>] [I<--current>] [I<--live>] [I<--config>]
Per convention on other commands, --current is mutually exclusive with --live or --config, so I'd show that part of the line as: [[I<--live>] [I<--config>] | [I<--current>]] ACK with nits fixed, and about time we had this useful command! (I tried, and failed to complete, something similar more than a year ago.) -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 2012年02月28日 02:41, Eric Blake wrote:
On 02/27/2012 05:11 AM, Osier Yang wrote:
One could use it to eject, insert, or update media of the CDROM or floppy drive. See the documents for more details.
s/documents/documentation/
--- tools/virsh.c | 142 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 33 ++++++++++++- 2 files changed, 172 insertions(+), 3 deletions(-)
+static const vshCmdOptDef opts_change_media[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"path", VSH_OT_DATA, VSH_OFLAG_REQ, N_("Fully-qualified path or " + "target of disk device")}, + {"source", VSH_OT_DATA, 0, N_("source of the media")}, + {"eject", VSH_OT_BOOL, 0, N_("Eject the media")}, + {"insert", VSH_OT_BOOL, 0, N_("Insert the media")}, + {"update", VSH_OT_BOOL, 0, N_("Update the media")},
I still wonder if --mode={eject|insert|update} would have been any easier to code, but that's just painting the bikeshed, so don't worry about it.
+static bool +cmdChangeMedia(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + const char *source = NULL; + const char *path = NULL; + const char *doc = NULL; + xmlNodePtr disk_node = NULL; + const char *disk_xml = NULL; + int flags = 0; + int config, live, current, force = 0;
s/int/bool/
+ int eject, insert, update = 0;
s/int/bool/
+ bool ret = false; + int prepare_type = 0; + const char *action = NULL; + + config = vshCommandOptBool(cmd, "config"); + live = vshCommandOptBool(cmd, "live"); + current = vshCommandOptBool(cmd, "current"); + force = vshCommandOptBool(cmd, "force"); + eject = vshCommandOptBool(cmd, "eject"); + insert = vshCommandOptBool(cmd, "insert"); + update = vshCommandOptBool(cmd, "update");
Particularly since you are assigning all 7 variables from vshCommandOptBool.
+ + if ((eject&& insert) || + (insert&& update) || + (eject&& update)) {
A shorter way to write this:
if (eject + insert + update> 1) {
Nice.
+ vshError(ctl, "%s", _("--eject, --insert, and --update must be specified " + "exclusively.")); + return false; + } + + if (eject) { + prepare_type = VSH_PREPARE_DISK_XML_EJECT; + action = "eject"; + } + + if (insert) { + prepare_type = VSH_PREPARE_DISK_XML_INSERT; + action = "insert"; + } + + if (update) { + prepare_type = VSH_PREPARE_DISK_XML_UPDATE; + action = "update"; + } + + /* Defaults to "update" */ + if (!eject&& !insert&& !update) { + prepare_type = VSH_PREPARE_DISK_XML_UPDATE; + action = "update";
You can combine these two clauses:
if (update || (!eject&& !insert)) {
Nice.
+ + if (virDomainUpdateDeviceFlags(dom, disk_xml, flags) != 0) { + vshError(ctl, _("Failed to %s media"), action);
Translators won't like this; there are languages where the spelling of the rest of the sentence depends on the gender of the word in 'action'. Better would be:
"Failed to complete '%s' action on media"
+ goto cleanup; + } + + vshPrint(ctl, _("succeeded to %s media\n"), action);
and again, better would be:
"Successfully completed '%s' action on media'
@@ -16705,6 +16846,7 @@ static const vshCmdDef domManagementCmds[] = { #ifndef WIN32 {"console", cmdConsole, opts_console, info_console, 0}, #endif + {"change-media", cmdChangeMedia, opts_change_media, info_change_media, 0},
Sorting - change-media comes _before_ console
+++ b/tools/virsh.pod @@ -1490,9 +1490,10 @@ from the domain. =item B<detach-interface> I<domain-id> I<type> [I<--mac mac>]
Detach a network interface from a domain. -I<type> can be either I<network> to indicate a physical network device or I<bridge> to indicate a bridge to a device. -It is recommended to use the I<mac> option to distinguish between the interfaces -if more than one are present on the domain. +I<type> can be either I<network> to indicate a physical network device or +I<bridge> to indicate a bridge to a device. It is recommended to use the +I<mac> option to distinguish between the interfaces if more than one are +present on the domain.
Unrelated hunk; for backport purposes, it would be nicer if you split this cleanup hunk into a separate (pre-approved) patch.
Ah, yeah, right.
=item B<update-device> I<domain-id> I<file> [I<--persistent>] [I<--force>]
@@ -1503,6 +1504,32 @@ option can be used to force device update, e.g., to eject a CD-ROM even if it is locked/mounted in the domain. See the documentation to learn about libvirt XML format for a device.
+=item B<change-media> I<domain-id> I<path> [I<--eject>] [I<--insert>] +[I<--update>] [I<source>] [I<--force>] [I<--current>] [I<--live>] [I<--config>]
Per convention on other commands, --current is mutually exclusive with --live or --config, so I'd show that part of the line as:
[[I<--live>] [I<--config>] | [I<--current>]]
ACK with nits fixed, and about time we had this useful command! (I tried, and failed to complete, something similar more than a year ago.)
Fixed the nits and pushed, thanks for the reviewing! Osier
participants (2)
-
Eric Blake
-
Osier Yang