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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]