Thanks all for the reply.
On 2024/4/24 03:15, Sean Christopherson wrote:
On Tue, Apr 23, 2024, Andrew Jones wrote:
On Tue, Apr 23, 2024 at 07:56:01AM -0700, Sean Christopherson wrote:
+others
On Tue, Apr 23, 2024, Markus Elfring wrote:
…
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?
Nah, don't bother with Fixes. OOM will cause the test to fail regardless, the
fact that it gets an assert instead a NULL pointer deref is nice to have, but by
no means does it fix a bug.
+++ 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?
Probably not, but there's also no reason to assume ENOMEM. TEST_ASSERT() spits
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;
})
Sounds good to me, but I'd call them test_malloc, test_calloc, etc. and
put them in include/test_util.h
Possibly terrible idea: what if we used kmalloc() and kcalloc()? K is for KVM :-)
I'am agree with that we should keep opening state for other memory
allocate calls as well.
I like test_* more than kvm_*, but I'm mildly concerned that readers will be
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.
It'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 '
ksft_exit_fail_*' 'ASSERT_*' 'CHECK*' or just use if statement.
Different part different developer has different usage habits.
We should think these status quo before doing sth.
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
And another question is if we add a new macro, whether these old usage
should be changed as well.
Thanks for your reply.
Looking forward to your reply.
