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

[Alexandru Elisei <alexandru.elisei@arm.com>]

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@arm.com>
> > > > ---
> > > >  configure | 36 ++++++++++++++++++++++++------------
> > > >  1 file changed, 24 insertions(+), 12 deletions(-)
> > > >
> > > 
> > > Reviewed-by: Andrew Jones <andrew.jones@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
}

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

Thanks,
Alex

> 
> Thanks,
> drew
>