[virt-tools-list] [virt-manager PATCH v2] cli: Fix Add --iothreads

[Cole Robinson]

Title should have 'Fix' removed

On 6/3/19 7:56 AM, Athina Plaskasoviti wrote:
> XML Mapping:
> 
> <domain>
> ...
>   <iothreads>X</iothreads>
> ...
> </domain>
> 
> Signed-off-by: Athina Plaskasoviti <athina.plaskasoviti at gmail.com>
> ---

When doing a v2, it's helpful to add a description here about what
changed between v1 and v2. Putting it after the --- means the text
doesn't end up in the commit message.

>  .../virt-install-singleton-config-2.xml       |  2 ++
>  tests/clitest.py                              |  1 +
>  virtinst/cli.py                               | 20 +++++++++++++++++++
>  virtinst/guest.py                             |  4 +++-
>  4 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/cli-test-xml/compare/virt-install-singleton-config-2.xml b/tests/cli-test-xml/compare/virt-install-singleton-config-2.xml
> index cd22f5ae..b680093c 100644
> --- a/tests/cli-test-xml/compare/virt-install-singleton-config-2.xml
> +++ b/tests/cli-test-xml/compare/virt-install-singleton-config-2.xml
> @@ -9,6 +9,7 @@
>        <libosinfo:os id="http://fedoraproject.org/fedora/unknown"/>
>      </libosinfo:libosinfo>
>    </metadata>
> +  <iothreads>4</iothreads>
>    <memory>1048576</memory>
>    <currentMemory>524288</currentMemory>
>    <blkiotune>
> @@ -221,6 +222,7 @@
>        <libosinfo:os id="http://fedoraproject.org/fedora/unknown"/>
>      </libosinfo:libosinfo>
>    </metadata>
> +  <iothreads>4</iothreads>
>    <memory>1048576</memory>
>    <currentMemory>524288</currentMemory>
>    <blkiotune>
> diff --git a/tests/clitest.py b/tests/clitest.py
> index ceb1eebd..a5762e50 100644
> --- a/tests/clitest.py
> +++ b/tests/clitest.py
> @@ -485,6 +485,7 @@ numa.cell1.distances.sibling0.id=0,numa.cell1.distances.sibling0.value=21,\
>  cell1.distances.sibling1.id=1,cell1.distances.sibling1.value=10,\
>  cache.mode=emulate,cache.level=3
>  --cputune vcpupin0.vcpu=0,vcpupin0.cpuset=0-3
> +--iothreads iothreads=4
>  --metadata title=my-title,description=my-description,uuid=00000000-1111-2222-3333-444444444444,genid=e9392370-2917-565e-692b-d057f46512d6
>  --boot cdrom,fd,hd,network,menu=off,loader=/foo/bar,emulator=/new/emu,bootloader=/new/bootld,rebootTimeout=3,initargs="foo=bar baz=woo"
>  --idmap uid_start=0,uid_target=1000,uid_count=10,gid_start=0,gid_target=1000,gid_count=10
> diff --git a/virtinst/cli.py b/virtinst/cli.py
> index 5accba8f..2003c54e 100644
> --- a/virtinst/cli.py
> +++ b/virtinst/cli.py
> @@ -771,6 +771,10 @@ def add_guest_xml_options(geng):
>      geng.add_argument("--cputune", action="append",
>          help=_("Tune CPU parameters for the domain process."))
>  
> +    ParserIOThreads.register()
> +    geng.add_argument("--iothreads", action="append",
> +        help=_("Tune IOThreads parameters for the domain process."))
> +

Can you move this up a bit so it's before all the *tune options, just so
those options stay logically grouped in --help output. Change the
description to _("Set domain <iothreads> configuration.")

>      ParserNumatune.register()
>      geng.add_argument("--numatune", action="append",
>          help=_("Tune NUMA policy for the domain process."))
> @@ -2010,6 +2014,22 @@ class ParserCputune(VirtCLIParser):
>                      find_inst_cb=cls.vcpu_find_inst_cb)
>  
>  
> +#######################
> +# --iothreads parsing #
> +#######################
> +
> +class ParserIOThreads(VirtCLIParser):
> +    cli_arg_name = "iothreads"
> +    guest_propname = "iothreads"
> +    stub_none = False
> +
> +    @classmethod
> +    def _init_class(cls, **kwargs):
> +        VirtCLIParser._init_class(**kwargs)
> +        # Options for IOThreads config
> +        cls.add_arg("iothreads", "iothreads")
> +
> +

I mentioned it in Fabiano's mail, but I think remove_first = "iothreads"
is good to have, so please add it back.

The 'stub_none = False' bit can be dropped. That's really only needed in
cases where we want to explicitly handle '--iothreads none' but that
doesn't apply here. Looks like there's a few other wrong cases in cli.py
though

>  ###################
>  # --vcpus parsing #
>  ###################
> diff --git a/virtinst/guest.py b/virtinst/guest.py
> index 5479d4e9..7684cdc7 100644
> --- a/virtinst/guest.py
> +++ b/virtinst/guest.py
> @@ -145,7 +145,7 @@ class Guest(XMLBuilder):
>      XML_NAME = "domain"
>      _XML_PROP_ORDER = [
>          "type", "name", "uuid", "genid", "genid_enable",
> -        "title", "description", "_metadata",
> +        "title", "description", "_metadata", "iothreads",
>          "maxMemory", "maxMemorySlots", "memory", "_currentMemory",
>          "blkiotune", "memtune", "memoryBacking",
>          "_vcpus", "vcpu_current", "vcpu_placement",
> @@ -179,6 +179,8 @@ class Guest(XMLBuilder):
>  
>      name = XMLProperty("./name")
>  
> +    iothreads = XMLProperty("./iothreads", is_int=True)
> +    

Looks like there's some extra whitespace here. './setup.py pylint' warns
about that for me (pycodestyle catches it), so make sure you are running
that! Also whatever editor you are using, google for 'trailing
whitespace', there's probably a way to highlight it visually so you
won't miss it.

Thanks
Cole