[libvirt] [PATCH 3/4] virsh: New command cmdChangeMedia
Osier Yang
jyang at redhat.com
Tue Feb 14 04:58:58 UTC 2012
On 02/14/2012 06:18 AM, Eric Blake wrote:
> On 02/03/2012 02:23 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.
>> ---
>> tools/virsh.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 134 insertions(+), 0 deletions(-)
>
> I would squash 3/4 and 4/4 into one patch (new commands should be
> committed with documentation).
>
>> +
>> + if ((eject&& insert) ||
>> + (insert&& update) ||
>> + (eject&& update)) {
>> + vshError(ctl, "%s", _("--eject, --insert, and --update must be specified "
>> + "exclusively."));
>> + return false;
>> + }
>> +
>> + if (!eject&& !insert&& !update) {
>> + vshError(ctl, "%s", _("One of --eject, --insert, and --update should be "
>> + "specified"));
>> + return false;
>> + }
>
> Seems reasonable.
>
>> +
>> + if (eject)
>> + prepare_flags |= VSH_PREPARE_DISK_XML_EJECT;
>> +
>> + if (insert)
>> + prepare_flags |= VSH_PREPARE_DISK_XML_INSERT;
>> +
>> + if (update)
>> + prepare_flags |= VSH_PREPARE_DISK_XML_UPDATE;
>
> But that means that you are using these as an enum, not as a bitmask to
> be OR'd together.
Yeah, it should be enum.
>
>> +
>> + if (insert&& !source) {
>> + vshError(ctl, "%s", _("No disk source specified for inserting"));
>> + goto cleanup;
>> + }
>
> Shouldn't you also error out if (eject&& source)?
"source" is just ignored if (eject).
>
>> + if (virDomainUpdateDeviceFlags(dom, disk_xml, flags) != 0) {
>> + vshError(ctl, "%s", _("Failed to insert media"));
>
> The error message is a bit misleading in the --eject case, since we
> didn't insert media, but removed it.
Ah, yes, will update.
>
> But overall, I think this is a long over-due command.
>
More information about the libvir-list
mailing list