Re: [PATCH 3/3] CI: Add {adl,zen3p}-pvshim-* tests

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Oct 22, 2024 at 02:25:54PM +0100, Andrew Cooper wrote:
> On 21/10/2024 11:28 pm, Stefano Stabellini wrote:
> > On Mon, 21 Oct 2024, Andrew Cooper wrote:
> >> diff --git a/automation/scripts/qubes-x86-64.sh 
> >> b/automation/scripts/qubes-x86-64.sh
> >> index 76fbafac84..95090eb12c 100755
> >> --- a/automation/scripts/qubes-x86-64.sh
> >> +++ b/automation/scripts/qubes-x86-64.sh
> >> @@ -44,6 +45,11 @@ echo \"${passed}\"
> >>  
> >>          if [ "${test_variant}" = "dom0pvh-hvm" ]; then
> >>              domU_type="hvm"
> >> +        elif [ "${test_variant}" = "pvshim" ]; then
> >> +            domU_type="pvh"
> > This is not necessary since PVH is already the default. In theory, it is
> > harmless, but it caused me to do a double-take because I initially
> > thought I was missing something, given that PVH is expected to be the
> > default at this point.
> 
> That was more fallout of refactoring.
> 
> My first attempt (which worked) involved writing a whole domU_config=
> block, and then I decided that was a bad thing.
> 
> Selecting PVShim without also forcing type to PVH is a little fragile if
> anyone changes the domU_type default.  I think it's cleaner to keep both
> together, even if it happens to be re-selecting the default.

IMO it's okay to leave re-setting the default here explicitly.

> >> +            domU_extra_config='
> >> +pvshim = 1
> >> +'
> > Is there a reason this cannot be:
> >
> >     domU_extra_config='pvshim = 1'
> >
> > ?
> >
> > These are just minor cosmetics.
> 
> Again, refactoring from before pulling out domU_type.
> 
> I'm not fussed - I can shrink this to one line.

Either is fine for me. The short one is more readable now, while the
long one is easier to extend in the future.

Anyway, for the series (c80e9241209cc9ed7f66c3f45275f837ddafc21c and
previous two):
Reviewed-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature