Re: [qemu-s390x] [PATCH v2 05/20] Acceptance tests: introduce arch parameter and attribute

[Cornelia Huck]

On Thu, 7 Feb 2019 13:22:10 -0500
Cleber Rosa <address@hidden> wrote:

> On 2/7/19 1:02 PM, Cleber Rosa wrote:
> > 
> > 
> > On 2/6/19 10:40 AM, Cornelia Huck wrote:  
> >> On Fri,  1 Feb 2019 19:55:55 -0500
> >> Cleber Rosa <address@hidden> wrote:
> >>  
> >>> It's useful to define the architecture that should be used in
> >>> situations such as:
> >>>  * the intended target of the QEMU binary to be used on tests
> >>>  * the architecture of code to be run within the QEMU binary, such
> >>>    as a kernel image or a full blown guest OS image  
> >>
> >> Thinking a bit more about this: These two are often, but not
> >> necessarily, the same. For example, starting a machine with the 64 bit
> >> variant of an architecture to run a guest with the 32 bit variant of
> >> that architecture might be a valid case.
> >>  
> > 
> > I agree with everything you said, and that's why I imagine "arch" being
> > used as a safe default type of parameter.
> > 
> > See, the QEMU binary can be influenced by the "arch" parameter, but can
> > still be *defined* by the "qemu_bin" parameter.  That's the same
> > approach I believe can be applied to the architecture of the guest code.
> >  When time comes, we may add a "guest_arch" parameter of sorts.  We
> > don't have that use case now, but I believe we'll be covered when it comes.

Ok, makes sense.

> >   
> >>>
> >>> This commit introduces both a test parameter and a test instance
> >>> attribute, that will contain such a value.
> >>>
> >>> Now, when the "arch" test parameter is given, it will influence the
> >>> selection of the default QEMU binary, if one is not given explicitly
> >>> by means of the "qemu_img" parameter.
> >>>
> >>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> >>> Signed-off-by: Cleber Rosa <address@hidden>
> >>> ---
> >>>  docs/devel/testing.rst                    | 17 +++++++++++++++++
> >>>  tests/acceptance/avocado_qemu/__init__.py | 14 +++++++++++---
> >>>  2 files changed, 28 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
> >>> index 44c9b3ae74..d37c4b0e77 100644
> >>> --- a/docs/devel/testing.rst
> >>> +++ b/docs/devel/testing.rst
> >>> @@ -689,6 +689,16 @@ vm
> >>>  A QEMUMachine instance, initially configured according to the given
> >>>  ``qemu_bin`` parameter.
> >>>  
> >>> +arch
> >>> +~~~~
> >>> +
> >>> +The architecture that will be used on a number of different
> >>> +scenarios.  For instance, when a QEMU binary is not explicitly given,
> >>> +the one selected will depend on this attribute.  
> >>
> >> This is probably a bit too vague. (What are "different scenarios"?)
> >>  
> > 
> > I believe it's kind of vague because I was thinking of both the
> > "framework level" code, and the possible uses in the tests.
> > 
> > At the "framework" level, it's only used for the case listed there
> > ("when a QEMU binary is not explicitly given...").
> >   
> >> Does it select anything else than the architecture of the vm that will
> >> be started?
> >>  
> > 
> > No, it doesn't.  The idea here was that tests may choose to use the same
> > attribute value in their own specific ("different") scenarios.
> > 
> > I can certainly make this clearer.
> >   
> 
> Hi Cornelia,
> 
> How does this sound?
> 
> ---

What about adding the following as a lead-in:

"The architecture can be used on different levels of the stack, e.g. by
the framework or by the test itself."

That would probably explain what you wanted to express with "different
scenarios".

> 
> The architecture that will influence the selection of a QEMU binary
> (when one is not explicitly given).

s/that will/will/ ?

> 
> Tests are also free to use this attribute value, for their own needs.
> A test may, for instance, use the same value when selecting the
> architecture of a kernel or disk image to boot a VM with.
> 
> The ``arch`` attribute will be set to the test parameter of the same
> name, and if one is not given explicitly, it will be set to ``None``.
> 
> ---
> 
> It uses your previous point about the 64/32 bit host/guest as an
> example.  Again, a test could use another parameter (not a Test class
> attribute) if it wants to use code for a different arch than the host.

Sounds good!