From 7e79e064ea778e2953c9f321a60aae47a08d32cb Mon Sep 17 00:00:00 2001
From: Cole Robinson <crobinso@redhat.com>
Date: Wed, 28 Aug 2024 11:43:36 -0400
Subject: [PATCH] Revert virt-xml multiple --edit support

This reverts b34ae0d0c883972d74deef6cdadb29bdd1bbe487 and
1fef5d8661118a4f00a8689b323d275144b39dbe.

Playing with this some more I found some deeper problems. For example

```console
[:~/src/virt-manager] (main) $ ./virt-xml test-for-virtxml --connect test://`pwd`/tests/data/testdriver/testsuite.xml --print-diff --edit 1 --video model=FOO --edit 2 --sound model=BAR
--- Original XML
+++ Altered XML
@@ -180,14 +180,14 @@
     <graphics type="vnc" port="-1" autoport="yes">
       <listen type="address"/>
     </graphics>
-    <sound model="sb16"/>
+    <sound model="BAR"/>
     <sound model="es1370"/>
     <sound model="ich6"/>
     <video>
       <model type="vmvga" vram="16384" heads="1" primary="yes"/>
     </video>
     <video>
-      <model type="cirrus" vram="16384" heads="3"/>
+      <model type="FOO" vram="16384" heads="3"/>
     </video>
     <hostdev mode="subsystem" type="usb" managed="yes">
       <source>
```

There's other weirdness too, though it's mostly strange plays on previous weird behavior

* `--edit --video model=vga --edit --sound model=ich9 --video model=qxl` works
* `--add-device --sound model=foo --video model=bar` was previously rejected but now works
* `--remove-device --video model=vmvga --sound model=sb16` was previously rejected by now works

Fixing all this is not trivial. So I think we need to revert and go back to the drawing board.
---
 man/virt-xml.rst                              |  4 -
 .../virt-xml-change-graphics-and-video.xml    | 26 ------
 .../cli/compare/virt-xml-multiple-edit.xml    | 13 ---
 tests/test_cli.py                             |  6 +-
 virtinst/virtxml.py                           | 87 +++++++------------
 5 files changed, 32 insertions(+), 104 deletions(-)
 delete mode 100644 tests/data/cli/compare/virt-xml-change-graphics-and-video.xml
 delete mode 100644 tests/data/cli/compare/virt-xml-multiple-edit.xml

diff --git a/man/virt-xml.rst b/man/virt-xml.rst
index db0ca5716..ddd586bb8 100644
--- a/man/virt-xml.rst
+++ b/man/virt-xml.rst
@@ -71,10 +71,6 @@ XML ACTIONS
     before applying the requested changes. This allows completely rebuilding
     an XML block. See EXAMPLES for some usage.
 
-    It is possible to edit multiple XML blocks but every XML block needs to
-    be paired with it's own ``--edit``. So for example you can edit graphics
-    and video using ``--edit --graphics vnc --edit --video virtio``.
-
     EDIT-OPTIONS examples:
 
     * ``--edit``
diff --git a/tests/data/cli/compare/virt-xml-change-graphics-and-video.xml b/tests/data/cli/compare/virt-xml-change-graphics-and-video.xml
deleted file mode 100644
index 571c3e690..000000000
--- a/tests/data/cli/compare/virt-xml-change-graphics-and-video.xml
+++ /dev/null
@@ -1,26 +0,0 @@
-     <emulator>/usr/bin/qemu-system-x86_64</emulator>
-     <controller type="usb" index="0" model="qemu-xhci" ports="15"/>
-     <controller type="virtio-serial" index="0"/>
--    <channel type="spicevmc">
--      <target type="virtio" name="com.redhat.spice.0"/>
--    </channel>
--    <graphics type="spice" autoport="yes">
-+    <graphics type="vnc" autoport="yes">
-       <listen type="address"/>
-       <image compression="off"/>
-       <gl enable="no"/>
-     </graphics>
--    <audio id="1" type="spice"/>
-     <video>
--      <model type="qxl" ram="65536" vram="65536" vgamem="16384" heads="1" primary="yes"/>
-+      <model type="virtio" vram="65536" heads="1" primary="yes"/>
-     </video>
--    <redirdev bus="usb" type="spicevmc">
--    </redirdev>
--    <redirdev bus="usb" type="spicevmc">
--    </redirdev>
-   </devices>
- </domain>
-
-Domain 'test-spice' defined successfully.
-Changes will take effect after the domain is fully powered off.
diff --git a/tests/data/cli/compare/virt-xml-multiple-edit.xml b/tests/data/cli/compare/virt-xml-multiple-edit.xml
deleted file mode 100644
index fa13e387f..000000000
--- a/tests/data/cli/compare/virt-xml-multiple-edit.xml
+++ /dev/null
@@ -1,13 +0,0 @@
-   <os>
-     <type arch="i686">hvm</type>
-     <boot dev="hd"/>
-+    <boot dev="network"/>
-   </os>
-   <clock offset="utc"/>
-   <on_poweroff>destroy</on_poweroff>
-@@
-   <on_crash>destroy</on_crash>
-   <devices>
-   </devices>
-+  <cpu mode="host-passthrough"/>
- </domain>
diff --git a/tests/test_cli.py b/tests/test_cli.py
index b6bc2184c..53a52a3e5 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -1373,9 +1373,7 @@ c.add_invalid("test --cpu host-passthrough", grep="One of --edit, ")  # conflict
 c.add_invalid("test --edit --add-device --disk path=foo", grep="Conflicting options --edit, --add-device")
 c.add_invalid("test --edit 0 --disk path=", grep="Invalid --edit option '0'")
 c.add_invalid("test --edit --hostdev driver_name=vfio", grep='No --hostdev objects found in the XML')
-c.add_invalid("test --edit --cpu host-passthrough --boot hd,network", grep="Each XML-ACTION requires one --edit option.")
-c.add_invalid("test --edit --cpu host-passthrough --edit", grep="Each XML-ACTION requires one --edit option.")
-c.add_compare("test --print-diff --edit --cpu host-passthrough --edit --boot hd,network", "multiple-edit")
+c.add_invalid("test --edit --cpu host-passthrough --boot hd,network", grep="Only one change operation may be specified")
 c.add_invalid("test --edit", grep="No change specified.")
 c.add_invalid("test --edit 2 --cpu host-passthrough", grep="'--edit 2' requested but there's only 1 --cpu object in the XML")
 c.add_invalid("test-for-virtxml --edit 5 --tpm /dev/tpm", grep="'--edit 5' requested but there's only 1 --tpm object in the XML")
@@ -1385,7 +1383,6 @@ c.add_invalid("test-for-virtxml --edit --graphics password=foo,keymap= --update
 c.add_invalid("--build-xml --memory 10,maxmemory=20", grep="--build-xml not supported for --memory")
 c.add_invalid("test-state-shutoff --edit sparse=no --disk path=blah", grep="Don't know how to match device type 'disk' property 'sparse'")
 c.add_invalid("test --add-device --xml ./@foo=bar", grep="--xml can only be used with --edit")
-c.add_invalid("test --edit --xml ./@foo=bar --edit --video model=virtio", grep="--xml cannot be used with any other XML-ACTION")
 c.add_invalid("test-for-virtxml --edit --boot refresh-machine-type=yes", grep="Don't know how to refresh")
 c.add_compare("test --print-xml --edit --vcpus 7", "print-xml")  # test --print-xml
 c.add_compare("--edit --cpu host-passthrough", "stdin-edit", input_file=(_VIRTXMLDIR + "virtxml-stdin-edit.xml"))  # stdin test
@@ -1506,7 +1503,6 @@ c.add_compare("--remove-device --hostdev mdev_b1ae8bf6_38b0_4c81_9d44_78ce3f5204
 c = vixml.add_category("edit/remove spice graphics", "test-spice --print-diff --define")
 c.add_compare("--edit --graphics type=vnc", "change-spice-to-vnc")
 c.add_compare("--remove-device --graphics type=spice", "remove-spice-graphics")
-c.add_compare("--edit --graphics type=vnc --edit --video model=virtio", "change-graphics-and-video")
 
 c = vixml.add_category("add/rm devices and start", "test-state-shutoff --print-diff --start")
 c.add_invalid("--add-device --pm suspend_to_disk=yes", grep="Cannot use --add-device with --pm")  # --add-device without a device
diff --git a/virtinst/virtxml.py b/virtinst/virtxml.py
index 18abd60ea..439f08930 100644
--- a/virtinst/virtxml.py
+++ b/virtinst/virtxml.py
@@ -115,9 +115,8 @@ def check_action_collision(options):
     collisions = []
     for cliname in actions:
         optname = cliname.replace("-", "_")
-        if getattr(options, optname) not in [False, -1, None]:
-            if cliname not in collisions:
-                collisions.append(cliname)
+        if getattr(options, optname) not in [False, -1]:
+            collisions.append(cliname)
 
     if len(collisions) == 0:
         fail(_("One of %s must be specified.") %
@@ -127,51 +126,31 @@ def check_action_collision(options):
              ", ".join(["--" + c for c in collisions]))
 
 
-def get_option_parsers(options):
-    parsers = []
-
+def check_xmlopt_collision(options):
+    collisions = []
     for parserclass in cli.VIRT_PARSERS + [cli.ParserXML]:
         if getattr(options, parserclass.cli_arg_name):
-            parsers.append(parserclass)
+            collisions.append(parserclass)
 
-    if len(parsers) == 0:
+    if len(collisions) == 0:
         fail(_("No change specified."))
+    if len(collisions) != 1:
+        fail(_("Only one change operation may be specified "
+               "(conflicting options %s)") %
+               [c.cli_flag_name() for c in collisions])
 
-    if cli.ParserXML in parsers and parsers.count(cli.ParserXML) != len(parsers):
-        fail(_("--xml cannot be used with any other XML-ACTION"))
-
-    return parsers
+    return collisions[0]
 
 
-def run_action(parsers, xmlobj, options, devs=None):
-    ret = []
-
-    edits = None
-    if options.edit:
-        edits = iter(options.edit)
-
-    for parserclass in parsers:
-        if options.edit:
-            ret.extend(action_edit(xmlobj, options, next(edits), parserclass))
-        elif options.add_device:
-            ret.extend(action_add_device(xmlobj, options, parserclass, devs))
-        elif options.remove_device:
-            ret.extend(action_remove_device(xmlobj, options, parserclass))
-        elif options.build_xml:
-            ret.extend(action_build_xml(options, parserclass, xmlobj))
-
-    return ret
-
-
-def action_edit(guest, options, edit, parserclass):
+def action_edit(guest, options, parserclass):
     if parserclass.guest_propname:
-        inst = _find_objects_to_edit(guest, "edit", edit, parserclass)
+        inst = _find_objects_to_edit(guest, "edit", options.edit, parserclass)
     else:
         inst = guest
-        if edit and edit != '1' and edit != 'all':
+        if options.edit and options.edit != '1' and options.edit != 'all':
             fail(_("'--edit %(option)s' doesn't make sense with "
                    "--%(objecttype)s, just use empty '--edit'") %
-                 {"option": edit,
+                 {"option": options.edit,
                   "objecttype": parserclass.cli_arg_name})
     if options.os_variant is not None:
         fail(_("--os-variant/--osinfo is not supported with --edit"))
@@ -319,7 +298,7 @@ def update_changes(domain, devs, action, confirm):
             print_stdout("")
 
 
-def prepare_changes(orig_xmlobj, options, parsers, devs=None):
+def prepare_changes(orig_xmlobj, options, parserclass, devs=None):
     """
     Parse the command line device/XML arguments, and apply the changes to
     a copy of the passed in xmlobj.
@@ -328,8 +307,8 @@ def prepare_changes(orig_xmlobj, options, parsers, devs=None):
     """
     origxml = orig_xmlobj.get_xml()
     xmlobj = orig_xmlobj.__class__(conn=orig_xmlobj.conn, parsexml=origxml)
-    has_edit = options.edit is not None
-    is_xmlcli = cli.ParserXML in parsers
+    has_edit = options.edit != -1
+    is_xmlcli = parserclass is cli.ParserXML
 
     if is_xmlcli and not has_edit:
         fail(_("--xml can only be used with --edit"))
@@ -339,15 +318,15 @@ def prepare_changes(orig_xmlobj, options, parsers, devs=None):
             devs = []
             cli.parse_xmlcli(xmlobj, options)
         else:
-            devs = run_action(parsers, xmlobj, options)
+            devs = action_edit(xmlobj, options, parserclass)
         action = "update"
 
     elif options.add_device:
-        devs = run_action(parsers, xmlobj, options, devs=devs)
+        devs = action_add_device(xmlobj, options, parserclass, devs)
         action = "hotplug"
 
     elif options.remove_device:
-        devs = run_action(parsers, xmlobj, options)
+        devs = action_remove_device(xmlobj, options, parserclass)
         action = "hotunplug"
 
     newxml = xmlobj.get_xml()
@@ -382,8 +361,7 @@ def parse_args():
         help=_("Domain name, id, or uuid"))
 
     actg = parser.add_argument_group(_("XML actions"))
-    actg.add_argument("--edit", nargs='?', default=None,
-                      action="append",
+    actg.add_argument("--edit", nargs='?', default=-1,
         help=_("Edit VM XML. Examples:\n"
         "--edit --disk ...     (edit first disk device)\n"
         "--edit 2 --disk ...   (edit second disk device)\n"
@@ -495,16 +473,13 @@ def main(conn=None):
     #   --build-xml
     check_action_collision(options)
 
-    parsers = get_option_parsers(options)
+    # Ensure there wasn't more than one device/xml config option
+    # specified. So reject '--disk X --network X'
+    parserclass = check_xmlopt_collision(options)
 
-    if options.edit and len(options.edit) != len(parsers):
-        fail(_("Each XML-ACTION requires one --edit option."))
-
-    if options.update:
-        for parserclass in parsers:
-            if not parserclass.guest_propname:
-                fail(_("Don't know how to --update for --%s") %
-                    (parserclass.cli_arg_name))
+    if options.update and not parserclass.guest_propname:
+        fail(_("Don't know how to --update for --%s") %
+             (parserclass.cli_arg_name))
 
     conn = cli.getConnection(options.connect, conn)
 
@@ -519,7 +494,7 @@ def main(conn=None):
     vm_is_running = bool(active_xmlobj)
 
     if options.build_xml:
-        devs = run_action(parsers, inactive_xmlobj, options)
+        devs = action_build_xml(options, parserclass, inactive_xmlobj)
         for dev in devs:
             # pylint: disable=no-member
             print_stdout(xmlutil.unindent_device_xml(dev.get_xml()))
@@ -532,7 +507,7 @@ def main(conn=None):
             fail_conflicting("--update", "--start")
         if vm_is_running:
             devs, action, dummy = prepare_changes(
-                    active_xmlobj, options, parsers)
+                    active_xmlobj, options, parserclass)
             update_changes(domain, devs, action, options.confirm)
             performed_update = True
         else:
@@ -544,7 +519,7 @@ def main(conn=None):
 
     original_xml = inactive_xmlobj.get_xml()
     devs, action, xmlobj_to_define = prepare_changes(
-            inactive_xmlobj, options, parsers, devs=devs)
+            inactive_xmlobj, options, parserclass, devs=devs)
     if not options.define:
         if options.start:
             start_domain_transient(conn, xmlobj_to_define, devs,