On Tue, Apr 23, 2024, Andrew Jones wrote:I'am agree with that we should keep opening state for other memory allocate calls as well.
On Tue, Apr 23, 2024 at 07:56:01AM -0700, Sean Christopherson wrote:Possibly terrible idea: what if we used kmalloc() and kcalloc()? K is for KVM :-)
+othersSounds good to me, but I'd call them test_malloc, test_calloc, etc. and
On Tue, Apr 23, 2024, Markus Elfring wrote:
…Nah, don't bother with Fixes. OOM will cause the test to fail regardless, the
This patch will add the malloc failure checking…
* Please use a corresponding imperative wording for the change description.
* Would you like to add the tag “Fixes” accordingly?
fact that it gets an assert instead a NULL pointer deref is nice to have, but by
no means does it fix a bug.
Probably not, but there's also no reason to assume ENOMEM. TEST_ASSERT() spits+++ b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c…
@@ -91,6 +91,7 @@ void test_vmx_nested_state(struct kvm_vcpu *vcpu)
const int state_sz = sizeof(struct kvm_nested_state) + getpagesize();
struct kvm_nested_state *state =
(struct kvm_nested_state *)malloc(state_sz);
+ TEST_ASSERT(state, "-ENOMEM when allocating kvm state");
Can “errno” be relevant for the error message construction?
out the actual errno, and we can just say something like "malloc() failed for
blah blah blah".
But rather than keeping playing whack-a-mole, what if we add macros to perform
allocations and assert on the result? I have zero interest in chasing down all
of the "unsafe" allocations, and odds are very good that we'll collectively fail
to enforce checking on new code.
E.g. something like (obviously won't compile, just for demonstration purposes)
#define kvm_malloc(x)
({
void *__ret;
__ret = malloc(x);
TEST_ASSERT(__ret, "Failed malloc(" #x ")\n");
__ret;
})
#define kvm_calloc(x, y)
({
void *__ret;
__ret = calloc(x, y);
TEST_ASSERT(__ret, "Failed calloc(" #x ", " #y ")\n");
__ret;
})
put them in include/test_util.h
I like test_* more than kvm_*, but I'm mildly concerned that readers will beIt's a good idea. The marco should be more versatile, cause we had many different way in selftests to check the null pointer or fail state, such as '
confused by "test", e.g. initially thinking the "test" means it's just "testing"
if allocation is possible.
The obvious counter-argument is that people might also get tripped by kmalloc(),
e.g. thinking that selftests is somehow doing a kernel allocation.
I almost wonder if we should just pick a prefix that's less obviously connected
to KVM and/or selftests, but unique and short.
Hmm, tmalloc(), i.e t[est]malloc()? tcalloc() gets a bit close to Google's
TCMalloc[*], but I suspect that any confusion would be entirely limited to
Googlers, and I'll volunteer us to suck it up and deal with it :-)
[*] https://github.com/google/tcmalloc