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

[Wainer dos Santos Moschetta <wainersm@redhat.com>]

Hi,

On 4/7/21 5:01 PM, Eduardo Habkost wrote:
> On Tue, Mar 23, 2021 at 05:01:09PM -0400, John Snow wrote:
>> 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.
> I don't have any specific suggestions on how the API should look
> like, but I'm having trouble understanding the documentation
> above.
>
> I don't know what "it will search and replace all occurrences"
> means.  Occurrences of what?
>
> I don't understand what "it is used the first value of the `arg`
> list" means, either.  I understand you are going to use the first
> value of the list, but you don't say what you are going to do
> with it.


The documentation was indeed confusing but, please, disregard it. Based 
on John's comments on this thread I decided to not introduce yet another 
specialized function to QEMUMachine class. Instead I added the "args" 
property so that users will have access to QEMUMachine._args to change 
it whatever they like. You will find that implemented on the v2 of this 
series:

'[PATCH v2 0/7] tests/acceptance: Handle tests with "cpu" tag'

Thanks!

- Wainer


>