Re: [PATCH 0/3] tests/acceptance: Handle tests with "cpu" tag

[John Snow <jsnow@redhat.com>]

On 3/17/21 3:16 PM, Wainer dos Santos Moschetta wrote:
> Added John and Eduardo,
> 
> On 3/9/21 3:52 PM, Cleber Rosa wrote:
>> On Wed, Feb 24, 2021 at 06:26:51PM -0300, Wainer dos Santos Moschetta 
>> wrote:
>>> Currently the acceptance tests tagged with "machine" have the "-M TYPE"
>>> automatically added to the list of arguments of the QEMUMachine object.
>>> In other words, that option is passed to the launched QEMU. On this
>>> series it is implemented the same feature but instead for tests marked
>>> with "cpu".
>>>
>> Good!
>>
>>> There is a caveat, however, in case the test needs additional 
>>> arguments to
>>> the CPU type they cannot be passed via tag, because the tags parser 
>>> split
>>> values by comma. For example, in 
>>> tests/acceptance/x86_cpu_model_versions.py,
>>> there are cases where:
>>>
>>>    * -cpu is set to 
>>> "Cascadelake-Server,x-force-features=on,check=off,enforce=off"
>>>    * if it was tagged like 
>>> "cpu:Cascadelake-Server,x-force-features=on,check=off,enforce=off"
>>>      then the parser would break it into 4 tags 
>>> ("cpu:Cascadelake-Server",
>>>      "x-force-features=on", "check=off", "enforce=off")
>>>    * resulting on "-cpu Cascadelake-Server" and the remaining 
>>> arguments are ignored.
>>>
>>> For the example above, one should tag it (or not at all) as 
>>> "cpu:Cascadelake-Server"
>>> AND self.vm.add_args('-cpu', 
>>> "Cascadelake-Server,x-force-features=on,check=off,enforce=off"),
>>> and that results on something like:
>>>
>>>    "qemu-system-x86_64 (...) -cpu Cascadelake-Server -cpu 
>>> Cascadelake-Server,x-force-features=on,check=off,enforce=off".
>>>
>> There are clearly two problems here:
>>
>> 1) the tag is meant to be succinct, so that it can be used by users
>>     selecting which tests to run.  At the same time, it's a waste
>>     to throw away the other information or keep it duplicate or
>>     incosistent.
>>
>> 2) QEMUMachine doesn't keep track of command line arguments
>>     (add_args() makes it pretty clear what's doing).  But, on this type
>>     of use case, a "set_args()" is desirable, in which case it would
>>     overwrite the existing arguments for a given command line option.
> 
> I like the idea of a "set_args()" to QEMUMachine as you describe above 
> but it needs further discussion because I can see at least one corner 
> case; for example, one can set the machine type as either -machine or 
> -M, then what key it should be searched-and-replaced (if any) on the 
> list of args?
> 
> Unlike your suggestion, I thought on implement the method to deal with a 
> single argument at time, as:
> 
>      def set_arg(self, arg: Union[str, list], value: str) -> None:
>          """
>          Set the value of an argument from the list of extra arguments 
> to be
>          given to the QEMU binary. If the argument does not exist then 
> it is
>          added to the list.
> 
>          If the ``arg`` parameter is a list then it will search and 
> replace all
>          occurencies (if any). Otherwise a new argument is added and it is
>          used the first value of the ``arg`` list.
>          """
>          pass
> 
> Does it sound good to you?
> 
> Thanks!
> 
> Wainer
> 

A little hokey, but I suppose that's true of our CLI interface in general.

I'd prefer not get into the business of building a "config" inside the 
python module if we can help it right now, but if "setting" individual 
args is something you truly need to do, I won't stand in the way.

Do what's least-gross.

--js

>>> QEMU is going to ignore the first -cpu argument. See the patch 0003 
>>> for a reference.
>>>
>> But this would still be creating a QEMU command line with multiple
>> '-cpu' arguments, right?  I understand this could be useful for
>> testing the behavior of the parameter parsing (if that's intended) but
>> it's bad practice to be generating incorrect command line in tests.
>>
>> Maybe just by tackling issue #2 this could be avoided.
>>
>> Cheers,
>> - Cleber.