[kvm-unit-tests PATCH v3 03/16] configure: Export TARGET unconditionally

[Andrew Jones]

On Thu, May 08, 2025 at 11:05:32AM +0100, Alexandru Elisei wrote:
> Hi Drew,
> 
> On Thu, May 08, 2025 at 11:39:54AM +0200, Andrew Jones wrote:
> > On Thu, May 08, 2025 at 09:52:38AM +0100, Alexandru Elisei wrote:
> > > Hi Drew,
> > > 
> > > On Wed, May 07, 2025 at 06:02:31PM +0200, Andrew Jones wrote:
> > > > On Wed, May 07, 2025 at 04:12:43PM +0100, Alexandru Elisei wrote:
> > > > > Only arm and arm64 are allowed to set --target to kvmtool; the rest of the
> > > > > architectures can only set --target to 'qemu', which is also the default.
> > > > > 
> > > > > Needed to make the changes necessary to add support for kvmtool to the test
> > > > > runner.
> > > > > 
> > > > > kvmtool also supports running the riscv tests, so it's not outside of the
> > > > > realm of the possibily for the riscv tests to get support for kvmtool.
> > > > > 
> > > > > Signed-off-by: Alexandru Elisei <alexandru.elisei at arm.com>
> > > > > ---
> > > > >  configure | 36 ++++++++++++++++++++++++------------
> > > > >  1 file changed, 24 insertions(+), 12 deletions(-)
> > > > >
> > > > 
> > > > Reviewed-by: Andrew Jones <andrew.jones at linux.dev>
> > > 
> > > Thank you for the review!
> > > 
> > > Just to be clear, you are ok with this happening because of the patch:
> > > 
> > > $ git pull
> > > $ make clean && make
> > > $ ./run_tests.sh
> > > scripts/runtime.bash: line 24: scripts/arch-run.bash: line 444: [: =: unary operator expected
> > > timeout -k 1s --foreground 90s /usr/bin/qemu-system-x86_64 --no-reboot -nodefaults -device pc-testdev -device isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc none -serial stdio -device pci-testdev -machine accel=kvm -kernel _NO_FILE_4Uhere_ 2 #  /tmp/tmp.bME9I2BZRG
> > > qemu-system-x86_64: 2: Could not open '2': No such file or directory
> > > scripts/arch-run.bash: line 19: 1: command not found: No such file or directory
> > > FAIL apic-split
> > > scripts/runtime.bash: line 24: scripts/arch-run.bash: line 444: [: =: unary operator expected
> > > timeout -k 1s --foreground 90s /usr/bin/qemu-system-x86_64 --no-reboot -nodefaults -device pc-testdev -device isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc none -serial stdio -device pci-testdev -machine accel=kvm -kernel _NO_FILE_4Uhere_ 1 #  /tmp/tmp.11und6qZbL
> > > qemu-system-x86_64: 1: Could not open '1': No such file or directory
> > > scripts/arch-run.bash: line 19: 1: command not found: No such file or directory
> > > FAIL ioapic-split
> > > [..]
> > > 
> > > That's because TARGET is missing from config.mak. If you're ok with the
> > > error, I'll make it clear in the commit message why this is happening.
> > >
> > 
> > It's not ideal, but I think it's pretty common to run configure before
> > make after an update to the git repo, so it's not horrible. However,
> > as you pointed out in your cover letter, this can be mitigated if we
> > use function wrappers for the associative array accesses, allowing
> > $TARGET to be checked before it's used. I'd prefer the function wrappers
> > anyway for readability reasons, so let's do that.
> 
> I'm all for the function wrappers, I was planning to reply to that comment
> later.
> 
> As to this patch, is this what you're thinking:
> 
> function vmm_optname_nr_cpus()
> {
> 	if [ -z $TARGET ]; then
> 		echo vmm_opts[qemu:nr_cpus]
> 	else
> 		echo vmm_opts[$TARGET:nr_cpus]
> 	fi
> }

Yup

> 
> But checking if $TARGET is defined makes this patch useless, and I would
> rather drop it if that's the case.

Sounds good.

Thanks,
drew