[virt-tools-list] [virt-manager PATCH 2/2] cli: fix cpu secure option to actually work

[Cole Robinson]

On 5/22/19 8:15 AM, Pavel Hrdina wrote:
> The 'secure' option is processed after the model is already set.
> CPU security options are resolved while setting CPU model so we need
> to know the 'secure' option value before we set the CPU model.
> 
> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---
> 

The cli options are applied to the XMLBuilder object in the order they
are listed in the parser class. So rather than peak into optdict, this
works in my testing:

diff --git a/virtinst/cli.py b/virtinst/cli.py
index e137fb14..7b27e390 100644
--- a/virtinst/cli.py
+++ b/virtinst/cli.py
@@ -1923,9 +1923,6 @@ class ParserCPU(VirtCLIParser):
         return cb(inst, *args, **kwargs)

     def set_model_cb(self, inst, val, virtarg):
-        if "secure" in self.optdict:
-            inst.secure = _on_off_convert("secure", self.optdict["secure"])
-
         if val == "host":
             val = inst.SPECIAL_MODE_HOST_MODEL
         if val == "none":
@@ -1954,11 +1951,11 @@ class ParserCPU(VirtCLIParser):
     @classmethod
     def _init_class(cls, **kwargs):
         VirtCLIParser._init_class(**kwargs)
+        cls.add_arg("secure", "secure", is_onoff=True)
         cls.add_arg("model", "model", cb=cls.set_model_cb)
         cls.add_arg("mode", "mode")
         cls.add_arg("match", "match")
         cls.add_arg("vendor", "vendor")
-        cls.add_arg("secure", "secure", is_onoff=True)
         cls.add_arg("cache.mode", "cache.mode")
         cls.add_arg("cache.level", "cache.level")

> The ideal fix would be to refactor CLI parsing to introduce some
> post-parse callback where we could properly set everything based
> on the user input.
> 

Yeah in general option ordering can't fix all issues, we can use
set_defaults to sort some complex deps out too. The big problem is using
virt-xml, lots of our option interactions are really only correctly
handled at XML build time and not XML edit time, but I haven't really
figured out a general way to resolve that

>  tests/cli-test-xml/compare/virt-install-cpu-disable-sec.xml | 4 ----
>  virtinst/cli.py                                             | 3 +++
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/cli-test-xml/compare/virt-install-cpu-disable-sec.xml b/tests/cli-test-xml/compare/virt-install-cpu-disable-sec.xml
> index de73803b..a86d6926 100644
> --- a/tests/cli-test-xml/compare/virt-install-cpu-disable-sec.xml
> +++ b/tests/cli-test-xml/compare/virt-install-cpu-disable-sec.xml
> @@ -14,8 +14,6 @@
>    </features>
>    <cpu mode="custom" match="exact">
>      <model>qemu64</model>
> -    <feature policy="require" name="spec-ctrl"/>
> -    <feature policy="require" name="ssbd"/>
>    </cpu>
>    <clock offset="utc">
>      <timer name="rtc" tickpolicy="catchup"/>
> @@ -63,8 +61,6 @@
>    </features>
>    <cpu mode="custom" match="exact">
>      <model>qemu64</model>
> -    <feature policy="require" name="spec-ctrl"/>
> -    <feature policy="require" name="ssbd"/>
>    </cpu>
>    <clock offset="utc">
>      <timer name="rtc" tickpolicy="catchup"/>
> diff --git a/virtinst/cli.py b/virtinst/cli.py
> index 5356e7b4..e137fb14 100644
> --- a/virtinst/cli.py
> +++ b/virtinst/cli.py
> @@ -1923,6 +1923,9 @@ class ParserCPU(VirtCLIParser):
>          return cb(inst, *args, **kwargs)
>  
>      def set_model_cb(self, inst, val, virtarg):
> +        if "secure" in self.optdict:
> +            inst.secure = _on_off_convert("secure", self.optdict["secure"])
> +
>          if val == "host":
>              val = inst.SPECIAL_MODE_HOST_MODEL
>          if val == "none":
> 


- Cole