Messages in this thread | | | Date | Wed, 24 Apr 2024 10:59:31 +0800 | Subject | Re: [PATCH] KVM: selftests: Add 'malloc' failure check in test_vmx_nested_state | From | Kunwu Chan <> |
| |
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.
| |