Re: [libvirt] [PATCH 3/3] virsh: New command cmdChangeMedia

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