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

Osier Yang jyang at redhat.com
Tue Feb 28 07:37:39 UTC 2012


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




More information about the libvir-list mailing list