Re: [kvm-unit-tests PATCH 2/2] s390x: Fix is_pv check in run script

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 10 Apr 2024 14:34:25 +1000
"Nicholas Piggin" <npiggin@xxxxxxxxx> wrote:

> On Mon Apr 8, 2024 at 9:36 PM AEST, Claudio Imbrenda wrote:
> > On Sat,  6 Apr 2024 22:24:54 +1000
> > Nicholas Piggin <npiggin@xxxxxxxxx> wrote:
> >  
> > > Shellcheck reports "is_pv references arguments, but none are ever
> > > passed." and suggests "use is_pv "$@" if function's $1 should mean
> > > script's $1."
> > > 
> > > The is_pv test does not evaluate to true for .pv.bin file names, only
> > > for _PV suffix test names. The arch_cmd_s390x() function appends
> > > .pv.bin to the file name AND _PV to the test name, so this does not
> > > affect run_tests.sh runs, but it might prevent PV tests from being
> > > run directly with the s390x-run command.
> > > 
> > > Reported-by: shellcheck SC2119, SC2120
> > > Reviewed-by: Andrew Jones <andrew.jones@xxxxxxxxx>
> > > Signed-off-by: Nicholas Piggin <npiggin@xxxxxxxxx>  
> >
> > Reviewed-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
> > Fixes: bcedc5a2 ("s390x: run PV guests with confidential guest enabled")  
> 
> Thanks.
> 
> > although tbh I would rewrite it to check a variable, something like:
> >
> > IS_PV=no
> > [ "${1: -7}" = ".pv.bin" -o "${TESTNAME: -3}" = "_PV" ] && IS_PV=yes  
> 
> I don't have a problem if you want to fix it a different way
> instead. I don't have a good way to test at the moment and
> this seemed the simplest fix. Shout out if you don't want it
> going upstream as is.

well, I did give you a r-b for the current version of your patch :)

it's not so important, as long as it works correctly 

> 
> Thanks,
> Nick
> 
> >  
> > > ---
> > >  s390x/run | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/s390x/run b/s390x/run
> > > index e58fa4af9..34552c274 100755
> > > --- a/s390x/run
> > > +++ b/s390x/run
> > > @@ -21,12 +21,12 @@ is_pv() {
> > >  	return 1
> > >  }
> > >  
> > > -if is_pv && [ "$ACCEL" = "tcg" ]; then
> > > +if is_pv "$@" && [ "$ACCEL" = "tcg" ]; then  
> >
> > if [ "$IS_PV" = "yes" -a "$ACCEL" = "tcg" ]; then
> >
> > etc...
> >  
> > >  	echo "Protected Virtualization isn't supported under TCG"
> > >  	exit 2
> > >  fi
> > >  
> > > -if is_pv && [ "$MIGRATION" = "yes" ]; then
> > > +if is_pv "$@" && [ "$MIGRATION" = "yes" ]; then
> > >  	echo "Migration isn't supported under Protected Virtualization"
> > >  	exit 2
> > >  fi
> > > @@ -34,12 +34,12 @@ fi
> > >  M='-machine s390-ccw-virtio'
> > >  M+=",accel=$ACCEL$ACCEL_PROPS"
> > >  
> > > -if is_pv; then
> > > +if is_pv "$@"; then
> > >  	M+=",confidential-guest-support=pv0"
> > >  fi
> > >  
> > >  command="$qemu -nodefaults -nographic $M"
> > > -if is_pv; then
> > > +if is_pv "$@"; then
> > >  	command+=" -object s390-pv-guest,id=pv0"
> > >  fi
> > >  command+=" -chardev stdio,id=con0 -device sclpconsole,chardev=con0"  
> 





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux