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.
+ + if (insert && !source) { + vshError(ctl, "%s", _("No disk source specified for inserting")); + goto cleanup; + }
Shouldn't you also error out if (eject && source)?
+ 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. But overall, I think this is a long over-due command. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org