Re: [PATCH v2 2/3] KVM: selftests: Add helper to read boolean module parameters

[Sean Christopherson <seanjc@google.com>]

On Wed, Sep 28, 2022, David Matlack wrote:
> @@ -114,6 +115,36 @@ void print_skip(const char *fmt, ...)
>  	puts(", skipping test");
>  }
>  
> +bool get_module_param_bool(const char *module_name, const char *param)
> +{
> +	const int path_size = 1024;
> +	char path[path_size];
> +	char value;
> +	FILE *f;
> +	int r;
> +
> +	r = snprintf(path, path_size, "/sys/module/%s/parameters/%s",
> +		     module_name, param);
> +	TEST_ASSERT(r < path_size,
> +		    "Failed to construct sysfs path in %d bytes.", path_size);
> +
> +	f = fopen(path, "r");

Any particular reason for using fopen()?  Oh, because that's what the existing
code does.  More below.

> +	TEST_ASSERT(f, "fopen(%s) failed", path);

I don't actually care myself, but for consistency this should probably be a
skip condition.  The easiest thing would be to use open_path_or_exit().

At that point, assuming read() instead of fread() does the right thin, that seems
like the easiest solution.

> +	TEST_FAIL("Unrecognized value: %c", value);

Maybe be slightly more verbose?  E.g.

	TEST_FAIL("Unrecognized value '%c' for boolean module param", value);

> +}
> +
>  bool thp_configured(void)
>  {
>  	int ret;
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 2e6e61bbe81b..522d3e2009fb 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -1294,20 +1294,9 @@ unsigned long vm_compute_max_gfn(struct kvm_vm *vm)
>  /* Returns true if kvm_intel was loaded with unrestricted_guest=1. */
>  bool vm_is_unrestricted_guest(struct kvm_vm *vm)
>  {
> -	char val = 'N';
> -	size_t count;
> -	FILE *f;
> -
>  	/* Ensure that a KVM vendor-specific module is loaded. */
>  	if (vm == NULL)
>  		close(open_kvm_dev_path_or_exit());
>  
> -	f = fopen("/sys/module/kvm_intel/parameters/unrestricted_guest", "r");
> -	if (f) {
> -		count = fread(&val, sizeof(char), 1, f);
> -		TEST_ASSERT(count == 1, "Unable to read from param file.");
> -		fclose(f);
> -	}
> -
> -	return val == 'Y';
> +	return get_module_param_bool("kvm_intel", "unrestricted_guest");

Since there are only three possible modules, what about providing wrappers to
handle "kvm", "kvm_amd", and "kvm_intel"?  I'm guessing we'll end up with wrappers
for each param we care about, but one fewer strings to get right would be nice.