Re: [PATCH v2 5/6] KVM: selftests: Add ucall_fmt2()

From: Sean Christopherson <seanjc@google.com>
To: Aaron Lewis <aaronlewis@google.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, jmattson@google.com
Subject: Re: [PATCH v2 5/6] KVM: selftests: Add ucall_fmt2()
Date: Mon, 5 Jun 2023 15:41:20 -0700	[thread overview]
Message-ID: <ZH5kkIWHCfDQy3EI@google.com> (raw)
In-Reply-To: <20230424225854.4023978-6-aaronlewis@google.com>

On Mon, Apr 24, 2023, Aaron Lewis wrote:
> Add a second ucall_fmt() function that takes two format strings instead
> of one.  This provides more flexibility because the string format in
> GUEST_ASSERT_FMT() is no linger limited to only using literals.

...

> -#define __GUEST_ASSERT_FMT(_condition, _condstr, _fmt, _args...)		  \
> -do {										  \
> -	if (!(_condition))							  \
> -		ucall_fmt(UCALL_ABORT,						  \
> -			  "Failed guest assert: " _condstr " at %s:%ld\n  " _fmt, \
> -			  , __FILE__, __LINE__, ##_args);			  \
> +#define __GUEST_ASSERT_FMT(_condition, _condstr, _fmt, _args...)	     \
> +do {									     \
> +	if (!(_condition))						     \
> +		ucall_fmt2(UCALL_ABORT,					     \
> +			   "Failed guest assert: " _condstr " at %s:%ld\n  ",\

I don't see any reason to add ucall_fmt2(), just do the string smushing in
__GUEST_ASSERT_FMT().  I doubt there will be many, if any, uses for this outside
of GUEST_ASSERT_FMT().  Even your test example is contrived, e.g. it would be
just as easy, and arguably more robusted, to #define the expected vs. actual formats
as it is to assign them to global variables.

In other words, this 

#define __GUEST_ASSERT_FMT(_condition, _str, _fmt, _args...)	     		\
do {										\
	char fmt_buffer[512];							\
										\
	if (!(_condition)) {							\
		kvm_snprintf(fmt_buffer, sizeof(fmt_buffer), "%s\n  %s",	\
			     "Failed guest assert: " _str " at %s:%ld", _fmt);	\
		ucall_fmt(UCALL_ABORT, fmt_buffer, __FILE__, __LINE__, ##_args);\
	}									\
} while (0)

is a preferable to copy+pasting an entirely new ucall_fmt2().  (Feel free to use
a different name for the on-stack array, e.g. just "fmt").

> +			   _fmt, __FILE__, __LINE__, ##_args);		     \
>  } while (0)
>  
>  #define GUEST_ASSERT_FMT(_condition, _fmt, _args...)	\
> diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c
> index c09e57c8ef77..d0f1ad6c0c44 100644
> --- a/tools/testing/selftests/kvm/lib/ucall_common.c
> +++ b/tools/testing/selftests/kvm/lib/ucall_common.c
> @@ -76,6 +76,30 @@ static void ucall_free(struct ucall *uc)
>  	clear_bit(uc - ucall_pool->ucalls, ucall_pool->in_use);
>  }
>  
> +void ucall_fmt2(uint64_t cmd, const char *fmt1, const char *fmt2, ...)
> +{
> +	const int fmt_len = 128;
> +	char fmt[fmt_len];

Just do 

	char fmt[128];

(or whatever size is chosen)

> +	struct ucall *uc;
> +	va_list va;
> +	int len;
> +
> +	len = kvm_snprintf(fmt, fmt_len, "%s%s", fmt1, fmt2);

and then here do sizeof(fmt).  It's self-documenting, and makes it really, really
hard to screw up and use the wrong format.

Regarding the size, can you look into why using 1024 for the buffer fails?  This
really should use the max allowed UCALL buffer size, but I'm seeing shutdowns when
pushing above 512 bytes (I didn't try to precisely find the threshold).  Selftests
are supposed to allocate 5 * 4KiB stacks, so the guest shouldn't be getting anywhere
near a stack overflow.

> +	if (len > fmt_len)

For KVM selftests use case, callers shouldn't need to sanity check, that should be
something kvm_snprintf() itself handles.  I'll follow-up in that patch.