virt-xml: Add --add-device and --remove-device options

9 files changed, 199 insertions, 19 deletions

diff --git a/tests/cli-test-xml/compare/virtxml-add-disk-basic.xml b/tests/cli-test-xml/compare/virtxml-add-disk-basic.xml
new file mode 100644
index 00000000..d5d7558f
--- /dev/null
+++ b/tests/cli-test-xml/compare/virtxml-add-disk-basic.xml
@@ -0,0 +1,14 @@
+--- Original XML
++++ Altered XML
+@@ -302,5 +302,9 @@
+         <protocol type="raw"/>
+       </backend>
+     </rng>
++    <disk type="file" device="disk">
++      <source file="/tmp/__virtinst_cli_exist1.img"/>
++      <target dev="vdf" bus="virtio"/>
++    </disk>
+   </devices>
+ </domain>
+
+Domain 'test-many-devices' defined successfully.
\ No newline at end of file
diff --git a/tests/cli-test-xml/compare/virtxml-add-host-device.xml b/tests/cli-test-xml/compare/virtxml-add-host-device.xml
new file mode 100644
index 00000000..4085a864
--- /dev/null
+++ b/tests/cli-test-xml/compare/virtxml-add-host-device.xml
@@ -0,0 +1,15 @@
+--- Original XML
++++ Altered XML
+@@ -302,5 +302,10 @@
+         <protocol type="raw"/>
+       </backend>
+     </rng>
++    <hostdev mode="subsystem" type="pci" managed="yes">
++      <source>
++        <address domain="0" bus="0" slot="25" function="0"/>
++      </source>
++    </hostdev>
+   </devices>
+ </domain>
+
+Domain 'test-many-devices' defined successfully.
\ No newline at end of file
diff --git a/tests/cli-test-xml/compare/virtxml-add-sound.xml b/tests/cli-test-xml/compare/virtxml-add-sound.xml
new file mode 100644
index 00000000..627ba39c
--- /dev/null
+++ b/tests/cli-test-xml/compare/virtxml-add-sound.xml
@@ -0,0 +1,11 @@
+--- Original XML
++++ Altered XML
+@@ -302,5 +302,6 @@
+         <protocol type="raw"/>
+       </backend>
+     </rng>
++    <sound model="pcspk"/>
+   </devices>
+ </domain>
+
+Domain 'test-many-devices' defined successfully.
\ No newline at end of file
diff --git a/tests/cli-test-xml/compare/virtxml-remove-disk-index.xml b/tests/cli-test-xml/compare/virtxml-remove-disk-index.xml
new file mode 100644
index 00000000..67a8f9c6
--- /dev/null
+++ b/tests/cli-test-xml/compare/virtxml-remove-disk-index.xml
@@ -0,0 +1,17 @@
+--- Original XML
++++ Altered XML
+@@ -83,12 +83,6 @@
+       <target dev="sdb" bus="scsi"/>
+       <readonly/>
+       <address type="drive" controller="0" bus="0" target="0" unit="1"/>
+-    </disk>
+-    <disk type="file" device="disk">
+-      <driver name="qemu" type="qcow2" cache="none"/>
+-      <source file="/tmp/foobar2"/>
+-      <target dev="vdc" bus="virtio"/>
+-      <shareable/>
+     </disk>
+     <disk type="block" device="disk">
+       <source dev="/dev/default-pool/overlay.img"/>
+
+Domain 'test-many-devices' defined successfully.
\ No newline at end of file
diff --git a/tests/cli-test-xml/compare/virtxml-remove-disk-path.xml b/tests/cli-test-xml/compare/virtxml-remove-disk-path.xml
new file mode 100644
index 00000000..e305e65e
--- /dev/null
+++ b/tests/cli-test-xml/compare/virtxml-remove-disk-path.xml
@@ -0,0 +1,29 @@
+--- Original XML
++++ Altered XML
+@@ -54,11 +54,6 @@
+   <on_crash>restart</on_crash>
+   <devices>
+     <emulator>/usr/lib/xen/bin/qemu-dm</emulator>
+-    <disk type="block" device="floppy">
+-      <source dev="/dev/null"/>
+-      <target dev="fda" bus="fdc"/>
+-      <address type="drive" controller="0" bus="0" target="0" unit="0"/>
+-    </disk>
+     <disk type="dir" device="floppy">
+       <source dir="/tmp"/>
+       <target dev="fdb" bus="fdc"/>
+@@ -72,12 +67,6 @@
+         <total_iops_sec>50</total_iops_sec>
+       </iotune>
+       <address type="drive" controller="0" bus="0" target="0" unit="0"/>
+-    </disk>
+-    <disk type="block" device="disk">
+-      <driver type="raw" cache="none"/>
+-      <source dev="/dev/null"/>
+-      <target dev="hdc" bus="ide"/>
+-      <address type="drive" controller="0" bus="1" target="0" unit="0"/>
+     </disk>
+     <disk type="block" device="cdrom">
+       <target dev="sdb" bus="scsi"/>
+
+Domain 'test-many-devices' defined successfully.
\ No newline at end of file
diff --git a/tests/cli-test-xml/compare/virtxml-remove-sound-model.xml b/tests/cli-test-xml/compare/virtxml-remove-sound-model.xml
new file mode 100644
index 00000000..8b75dd55
--- /dev/null
+++ b/tests/cli-test-xml/compare/virtxml-remove-sound-model.xml
@@ -0,0 +1,12 @@
+--- Original XML
++++ Altered XML
+@@ -254,7 +254,6 @@
+     </graphics>
+     <sound model="sb16"/>
+     <sound model="es1370"/>
+-    <sound model="ich6"/>
+     <video>
+       <model type="vmvga" vram="9216" heads="1"/>
+     </video>
+
+Domain 'test-many-devices' defined successfully.
\ No newline at end of file
diff --git a/tests/cli-test-xml/compare/virtxml-remove-video-all.xml b/tests/cli-test-xml/compare/virtxml-remove-video-all.xml
new file mode 100644
index 00000000..dbb2dca7
--- /dev/null
+++ b/tests/cli-test-xml/compare/virtxml-remove-video-all.xml
@@ -0,0 +1,17 @@
+--- Original XML
++++ Altered XML
+@@ -255,12 +255,6 @@
+     <sound model="sb16"/>
+     <sound model="es1370"/>
+     <sound model="ich6"/>
+-    <video>
+-      <model type="vmvga" vram="9216" heads="1"/>
+-    </video>
+-    <video>
+-      <model type="cirrus" vram="10240" heads="3"/>
+-    </video>
+     <hostdev mode="subsystem" type="usb" managed="yes">
+       <source>
+         <vendor id="0x04b3"/>
+
+Domain 'test-many-devices' defined successfully.
\ No newline at end of file
diff --git a/tests/clitest.py b/tests/clitest.py
index 032bd106..6f8969ad 100644
--- a/tests/clitest.py
+++ b/tests/clitest.py
@@ -805,6 +805,16 @@ c.add_compare("--edit --cpu host-passthrough,clearxml", "virtxml-edit-clear-cpu"
 c.add_compare("--edit --clock offset=utc,clearxml", "virtxml-edit-clear-clock")
 c.add_compare("--edit --disk /foo/bar,target=fda,bus=fdc,device=floppy,clearxml", "virtxml-edit-clear-disk")
 
+c = vixml.add_category("add/rm devices", "--domain test-many-devices --print-diff --define")
+c.add_invalid("--add-device --security foo")  # --add-device without a device
+c.add_invalid("--remove-device --clock utc")  # --remove-device without a dev
+c.add_compare("--add-device --host-device net_00_1c_25_10_b1_e4", "virtxml-add-host-device")
+c.add_compare("--add-device --soundhw pcspk", "virtxml-add-sound")
+c.add_compare("--add-device --disk %(EXISTIMG1)s,bus=virtio,target=vdf", "virtxml-add-disk-basic")
+c.add_compare("--remove-device --soundhw ich6", "virtxml-remove-sound-model")
+c.add_compare("--remove-device --disk 6", "virtxml-remove-disk-index")
+c.add_compare("--remove-device --disk /dev/null", "virtxml-remove-disk-path")
+c.add_compare("--remove-device --video all", "virtxml-remove-video-all")
 
 
 vimag = App("virt-image")
diff --git a/virt-xml b/virt-xml
index f75fdfed..96b729ef 100755
--- a/virt-xml
+++ b/virt-xml
@@ -27,6 +27,7 @@ import libvirt
 
 import virtinst
 from virtinst import cli
+from virtinst import util
 from virtinst.cli import fail, print_stdout, print_stderr
 
 
@@ -106,19 +107,19 @@ def get_domain_and_guest(conn, domstr):
 # Change logic #
 ################
 
-def _find_devices_to_edit(guest, options, parserobj):
+def _find_devices_to_edit(guest, action_name, editval, parserobj):
     devlist = guest.get_devices(parserobj.devclass.virtual_device_type)
     idx = None
 
-    if options.edit is None:
+    if editval is None:
         idx = 1
-    elif (options.edit.isdigit() or
-          options.edit.startswith("-") and options.edit[1:].isdigit()):
-        idx = int(options.edit)
+    elif (editval.isdigit() or
+          editval.startswith("-") and editval[1:].isdigit()):
+        idx = int(editval)
 
     if idx is not None:
         if idx == 0:
-            fail(_("Invalid --edit option '%s'") % options.edit)
+            fail(_("Invalid --edit option '%s'") % editval)
 
         if not devlist:
             fail(_("No --%s devices found in the XML") %
@@ -131,24 +132,36 @@ def _find_devices_to_edit(guest, options, parserobj):
         if idx > 0:
             idx -= 1
         inst = devlist[idx]
-    elif options.edit == "all":
+    elif editval == "all":
         inst = devlist[:]
     else:
-        inst = parserobj.lookup_device_from_option_string(guest, options.edit)
+        inst = parserobj.lookup_device_from_option_string(guest, editval)
         if not inst:
-            fail(_("No matching devices found for --edit %s") % options.edit)
+            fail(_("No matching devices found for --%s %s") %
+                 (action_name, editval))
 
     return inst
 
 
-def change_xml(guest, options, parsermap):
-    # XXX: Make sure actions don't conflict
-    # XXX: Make sure XML options don't conflict
-    # XXX: Find a way to factor out whatever defaults there are
-    if options.edit is -1:
-        fail("--edit must be specified")
+def check_action_collision(options):
+    actions = ["edit", "add-device", "remove-device"]
 
     collisions = []
+    for cliname in actions:
+        optname = cliname.replace("-", "_")
+        if getattr(options, optname) not in [False, -1]:
+            collisions.append(cliname)
+
+    if len(collisions) == 0:
+        fail(_("One of %s must be specified.") %
+             ", ".join(["--" + c for c in actions]))
+    if len(collisions) > 1:
+        fail(_("Conflicting options %s") %
+             ", ".join(["--" + c for c in collisions]))
+
+
+def check_xmlopt_collision(options, parsermap):
+    collisions = []
     for option_variable_name, parserobj in parsermap.items():
         if getattr(options, option_variable_name):
             collisions.append(parserobj)
@@ -159,10 +172,13 @@ def change_xml(guest, options, parsermap):
         fail(_("Only one change operation may be specified "
                "(conflicting options %s)") %
                ["--" + c.cli_arg_name for c in collisions])
-    parserobj = collisions[0]
 
+    return collisions[0]
+
+
+def action_edit(guest, options, parsermap, parserobj):
     if parserobj.devclass:
-        inst = _find_devices_to_edit(guest, options, parserobj)
+        inst = _find_devices_to_edit(guest, "edit", options.edit, parserobj)
     else:
         inst = guest
         if options.edit and options.edit != '1' and options.edit != 'all':
@@ -173,6 +189,25 @@ def change_xml(guest, options, parsermap):
     cli.parse_option_strings(parsermap, options, guest, inst, update=True)
 
 
+def action_add_device(guest, options, parsermap, parserobj):
+    if not parserobj.devclass:
+        fail(_("Cannot use --add-device with --%s") % parserobj.cli_arg_name)
+    cli.parse_option_strings(parsermap, options, guest, None)
+
+
+def action_remove_device(guest, options, parsermap, parserobj):
+    ignore = parsermap
+    if not parserobj.devclass:
+        fail(_("Cannot use --remove-device with --%s") %
+             parserobj.cli_arg_name)
+
+    devs = _find_devices_to_edit(guest, "remove-device",
+        getattr(options, parserobj.option_variable_name)[-1], parserobj)
+
+    for dev in util.listify(devs):
+        guest.remove_device(dev)
+
+
 #######################
 # CLI option handling #
 #######################
@@ -191,7 +226,19 @@ def parse_args():
     actg = parser.add_argument_group(_("Action Options"))
     actg.add_argument("--domain", help=_("Domain name, id, or uuid"))
     actg.add_argument("--edit", nargs='?', default=-1,
-                      help=_("Edit VM XML"))
+        help=_("Edit VM XML. Examples:\n"
+        "--edit --disk ...     (edit first disk device)\n"
+        "--edit 2 --disk ...   (edit second disk device)\n"
+        "--edit all --disk ... (edit all disk devices)\n"
+        "--edit target=hda --disk ... (edit disk 'hda')\n"))
+    actg.add_argument("--remove-device", action="store_true",
+        help=_("Remove specified device. Examples:\n"
+        "--remove-device --disk 1 (remove first disk)\n"
+        "--remove-device --disk all (remove all disks)\n"
+        "--remove-device --disk /some/path"))
+    actg.add_argument("--add-device", action="store_true",
+        help=_("Add specified device. Example:\n"
+        "--add-device --disk ..."))
 
     g = parser.add_argument_group(_("XML options"))
     cli.add_disk_option(g)
@@ -248,7 +295,15 @@ def main(conn=None):
     # XXX: do we ever need the domain?
     ignore = domain
 
-    change_xml(guest, options, parsermap)
+    check_action_collision(options)
+    parserobj = check_xmlopt_collision(options, parsermap)
+
+    if options.edit != -1:
+        action_edit(guest, options, parsermap, parserobj)
+    elif options.add_device:
+        action_add_device(guest, options, parsermap, parserobj)
+    elif options.remove_device:
+        action_remove_device(guest, options, parsermap, parserobj)
 
     newxml = guest.get_xml_config()
     diff = get_diff(origxml, newxml)