Re: [PATCH v2] kvm: selftests: Add KVM_CAP_MAX_VCPU_ID cap test

[Sean Christopherson <seanjc@google.com>]

On Tue, May 03, 2022, Zeng Guang wrote:
> +int main(int argc, char *argv[])
> +{
> +	struct kvm_vm *vm;
> +	struct kvm_enable_cap cap = { 0 };
> +	int ret;
> +
> +	vm = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
> +
> +	/* Get KVM_CAP_MAX_VCPU_ID cap supported in KVM */
> +	ret = vm_check_cap(vm, KVM_CAP_MAX_VCPU_ID);

Because it's trivial to do so, and will avoid a hardcoded "max", what about looping
over all possible values?  And then some arbitrary number of the max?

	max_nr_vcpus = vm_check_cap(vm, KVM_CAP_MAX_VCPU_ID);
	TEST_ASSERT(max_nr_vcpus > ???, ...)

	for (i = 0; i < max_nr_vcpus; i++) 
		vm = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);

		vm_set_max_nr_vcpus(vm, 0);
		vm_create_invalid_vcpu(vm, 0);

		vm_set_max_nr_vcpus(vm, i);
		vm_set_max_nr_vcpus(vm, i);
		vm_create_invalid_vcpu(vm, i);
		vm_create_invalid_vcpu(vm, i + 1);

		vm_set_invalid_max_nr_vcpus(vm, 0);
		vm_set_invalid_max_nr_vcpus(vm, i + 1);
		vm_set_invalid_max_nr_vcpus(vm, i - 1);
		vm_set_invalid_max_nr_vcpus(vm, max_nr_vcpus);

		close(vm->fd);
	}

	for ( ; i < max_nr_vcpus + 100; i++) {
		vm = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);

		vm_set_invalid_max_nr_vcpus(vm, i);

		close(vm->fd);
	}
> +
> +	/* Check failure if set KVM_CAP_MAX_VCPU_ID beyond KVM cap */
> +	cap.cap = KVM_CAP_MAX_VCPU_ID;
> +	cap.args[0] = ret + 1;
> +	ret = ioctl(vm->fd, KVM_ENABLE_CAP, &cap);

A helper or two to set the cap would be, uh, helpful :-)  See above for ideas.

> +	TEST_ASSERT(ret < 0,
> +		    "Unexpected success to enable KVM_CAP_MAX_VCPU_ID"
> +		    "beyond KVM cap!\n");

Please don't wrap quoted strings.  Shorten the string and/or let the line run long.
For the string/message, prioritize information that the user _can't_ get from looking
at the code, and info that is highly relevant to the expectations.  E.g. print the
the return value, the errno, and the allegedly bad value.

It's definitely helpful to provide context too (KVM Unit Tests drive me bonkers for
their terse messages), but for cases like this, it's redundant.  "Unexpected success"
is redundant because the "ret < 0" conveys that failure was expected, and hopefully
most people will intuit that the test was trying "to enable" KVM_CAP_MAX_VCPU_ID.
If not, a quick glance at the code (file and line provided) will give them that info.

E.g. assuming this ends up in a helper, something like

	TEST_ASSERT(ret == -1 && errno == EINVAL,
		    "KVM_CAP_MAX_VCPU_ID bug, max ID = %d, ret = %d, errno = %d (%s),
		    max_id, errno, strerror(errno));

IMO it's worth checking errno to reduce the probability of false pass, e.g. if the
ioctl() is rejected for some other reason due to a test bug.

> +
> +	/* Check success if set KVM_CAP_MAX_VCPU_ID */
> +	cap.args[0] = MAX_VCPU_ID;
> +	ret = ioctl(vm->fd, KVM_ENABLE_CAP, &cap);
> +	TEST_ASSERT(ret == 0,
> +		    "Unexpected failure to enable KVM_CAP_MAX_VCPU_ID!\n");