| Message ID | 20240423073952.2001989-1-chentao@kylinos.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftests: Add 'malloc' failure check in test_vmx_nested_state | expand |
On 4/23/24 12:39 PM, Kunwu Chan wrote: > There is a 'malloc' call in test_vmx_nested_state function, which can > be unsuccessful. This patch will add the malloc failure checking > to avoid possible null dereference and give more information > about test fail reasons. LGTM Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > > Signed-off-by: Kunwu Chan <chentao@kylinos.cn> > --- > tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c > index 67a62a5a8895..18afc2000a74 100644 > --- a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c > +++ 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"); > > /* The format must be set to 0. 0 for VMX, 1 for SVM. */ > set_default_vmx_state(state, state_sz);
… > 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? … > +++ 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? Regards, Markus
+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; })
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 Thanks, drew
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 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. 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
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.
On Tue, Apr 23, 2024 at 12:15:47PM -0700, 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 :-) That's a legit terrible idea... It probably would trigger more static checker warnings because the general policy is kmalloc() is kernel code and we *have* to test for errors. To be honest, I would have just rejected the first patch. You obviously know this and have said this earlier in the thread but just for the other people, this is a userspace test that runs for a short time and then exits. If it gets killed because we don't have enough memory that's fine. It would be better to just fix the static checker to not print pointless warnings or educate people to ignore warnings like this. Creating the test_malloc() to silence the warning also seems like an okay idea as well. regards, dan carpenter
On Tue, Apr 23, 2024 at 12:15:47PM -0700, Sean Christopherson wrote: ... > I almost wonder if we should just pick a prefix that's less obviously connected > to KVM and/or selftests, but unique and short. > How about kvmsft_ ? It's based on the ksft_ prefix of kselftest.h. Maybe it's too close to ksft though and would be confusing when using both in the same test? I'm not a huge fan of capital letters, but we could also do something like MALLOC()/CALLOC(). Eh, I don't know. Naming is hard. Thanks, drew
On Wed, Apr 24, 2024, Dan Carpenter wrote: > On Tue, Apr 23, 2024 at 12:15:47PM -0700, 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 :-) > > That's a legit terrible idea... It probably would trigger more static > checker warnings because the general policy is kmalloc() is kernel code > and we *have* to test for errors. Roger that. > To be honest, I would have just rejected the first patch. You > obviously know this and have said this earlier in the thread but just > for the other people, this is a userspace test that runs for a short > time and then exits. If it gets killed because we don't have enough > memory that's fine. It would be better to just fix the static checker > to not print pointless warnings or educate people to ignore warnings > like this. This particular patch may have been motiviated by a static checker, but I doubt static checkers are responsible for all of the many sanity checks on malloc() in KVM selftests. And while I agree that the sanity checks don't and much value, deleting the existing checks and preventing checks from being reintroduced would be a never ending battle. > Creating the test_malloc() to silence the warning also seems like an > okay idea as well. Yeah, it's not exactly my first choice, but the reality is that people write KVM elftests by copying an existing test (often literally), and so the best way to educate developers on the preferred approach/style is to have all existing code adhere to a single approach/style.
On Wed, Apr 24, 2024, Andrew Jones wrote: > On Tue, Apr 23, 2024 at 12:15:47PM -0700, Sean Christopherson wrote: > ... > > I almost wonder if we should just pick a prefix that's less obviously connected > > to KVM and/or selftests, but unique and short. > > > > How about kvmsft_ ? It's based on the ksft_ prefix of kselftest.h. Maybe > it's too close to ksft though and would be confusing when using both in > the same test? I would prefer something short, and for whatever reason I have a mental block with ksft. I always read it as "k soft", which is completely nonsensical :-) > I'm not a huge fan of capital letters, but we could also do something like > MALLOC()/CALLOC(). Hmm, I'm not usually a fan either, but that could actually work quite well in this case. It would be quite intuitive, easy to visually parse whereas tmalloc() vs malloc() kinda looks like a typo, and would more clearly communicate that they're macros.
Hey, On Wed, Apr 24, 2024 at 07:51:44AM -0700, Sean Christopherson wrote: > On Wed, Apr 24, 2024, Andrew Jones wrote: > > On Tue, Apr 23, 2024 at 12:15:47PM -0700, Sean Christopherson wrote: > > ... > > > I almost wonder if we should just pick a prefix that's less obviously connected > > > to KVM and/or selftests, but unique and short. > > > > > > > How about kvmsft_ ? It's based on the ksft_ prefix of kselftest.h. Maybe > > it's too close to ksft though and would be confusing when using both in > > the same test? > > I would prefer something short, and for whatever reason I have a mental block > with ksft. I always read it as "k soft", which is completely nonsensical :-) I despise brevity in tests, so my strong preference is to use some form of 'namespaced' helper. Perhaps others have better memory than I do, but I'm quick to forget the selftests library and find the more verbose / obvious function names helpful for jogging my memory. > > I'm not a huge fan of capital letters, but we could also do something like > > MALLOC()/CALLOC(). > > Hmm, I'm not usually a fan either, but that could actually work quite well in this > case. It would be quite intuitive, easy to visually parse whereas tmalloc() vs > malloc() kinda looks like a typo, and would more clearly communicate that they're > macros. Ooo, don't leave me out on the bikeshedding! How about TEST_MALLOC() / TEST_CALLOC(). It is vaguely similar to TEST_ASSERT(), which I'd hope would give the impression that an assertion is lurking below.
On Wed, Apr 24, 2024, Oliver Upton wrote: > Hey, > > On Wed, Apr 24, 2024 at 07:51:44AM -0700, Sean Christopherson wrote: > > On Wed, Apr 24, 2024, Andrew Jones wrote: > > > On Tue, Apr 23, 2024 at 12:15:47PM -0700, Sean Christopherson wrote: > > > ... > > > > I almost wonder if we should just pick a prefix that's less obviously connected > > > > to KVM and/or selftests, but unique and short. > > > > > > > > > > How about kvmsft_ ? It's based on the ksft_ prefix of kselftest.h. Maybe > > > it's too close to ksft though and would be confusing when using both in > > > the same test? > > > > I would prefer something short, and for whatever reason I have a mental block > > with ksft. I always read it as "k soft", which is completely nonsensical :-) > > I despise brevity in tests, so my strong preference is to use some form > of 'namespaced' helper. Perhaps others have better memory than > I do, but I'm quick to forget the selftests library and find the more > verbose / obvious function names helpful for jogging my memory. Hmm, I generally agree, but in this case I think there's value in having the names *not* stand out, because they really are uninteresting and would ideally blend in. I can't envision a scenario where we don't want to assert on an OOM, i.e. there should never be a need to use a raw malloc(), and so I don't see much value in making it obvious that the call sites are doing something special. > > > I'm not a huge fan of capital letters, but we could also do something like > > > MALLOC()/CALLOC(). > > > > Hmm, I'm not usually a fan either, but that could actually work quite well in this > > case. It would be quite intuitive, easy to visually parse whereas tmalloc() vs > > malloc() kinda looks like a typo, and would more clearly communicate that they're > > macros. > > Ooo, don't leave me out on the bikeshedding! How about TEST_MALLOC() / > TEST_CALLOC(). It is vaguely similar to TEST_ASSERT(), which I'd hope > would give the impression that an assertion is lurking below. Yeah, but it could also give the false impression that the macro does something fancier, e.g. this makes me want to peek at TEST_MALLOC() to see what it's doing cpuid = TEST_MALLOC(kvm_cpuid2_size(nr_entries)); whereas this isn't quite enough to pique my curiosity. cpuid = MALLOC(kvm_cpuid2_size(nr_entries)); So I have a slight preference for just MALLOC()/CALLOC(), but I'm also ok with a TEST_ prefix, my brain can adapt. One of those two flavors has my vote.
Thanks for reply. On 2024/4/26 00:25, Sean Christopherson wrote: > On Wed, Apr 24, 2024, Oliver Upton wrote: >> Hey, >> >> On Wed, Apr 24, 2024 at 07:51:44AM -0700, Sean Christopherson wrote: >>> On Wed, Apr 24, 2024, Andrew Jones wrote: >>>> On Tue, Apr 23, 2024 at 12:15:47PM -0700, Sean Christopherson wrote: >>>> ... >>>>> I almost wonder if we should just pick a prefix that's less obviously connected >>>>> to KVM and/or selftests, but unique and short. >>>>> >>>> How about kvmsft_ ? It's based on the ksft_ prefix of kselftest.h. Maybe >>>> it's too close to ksft though and would be confusing when using both in >>>> the same test? >>> I would prefer something short, and for whatever reason I have a mental block >>> with ksft. I always read it as "k soft", which is completely nonsensical :-) >> I despise brevity in tests, so my strong preference is to use some form >> of 'namespaced' helper. Perhaps others have better memory than >> I do, but I'm quick to forget the selftests library and find the more >> verbose / obvious function names helpful for jogging my memory. > Hmm, I generally agree, but in this case I think there's value in having the > names *not* stand out, because they really are uninteresting and would ideally > blend in. I can't envision a scenario where we don't want to assert on an OOM, > i.e. there should never be a need to use a raw malloc(), and so I don't see much > value in making it obvious that the call sites are doing something special. > >>>> I'm not a huge fan of capital letters, but we could also do something like >>>> MALLOC()/CALLOC(). >>> Hmm, I'm not usually a fan either, but that could actually work quite well in this >>> case. It would be quite intuitive, easy to visually parse whereas tmalloc() vs >>> malloc() kinda looks like a typo, and would more clearly communicate that they're >>> macros. >> Ooo, don't leave me out on the bikeshedding! How about TEST_MALLOC() / >> TEST_CALLOC(). It is vaguely similar to TEST_ASSERT(), which I'd hope >> would give the impression that an assertion is lurking below. > Yeah, but it could also give the false impression that the macro does something > fancier, e.g. this makes me want to peek at TEST_MALLOC() to see what it's doing > > cpuid = TEST_MALLOC(kvm_cpuid2_size(nr_entries)); > > whereas this isn't quite enough to pique my curiosity. > > cpuid = MALLOC(kvm_cpuid2_size(nr_entries)); > > So I have a slight preference for just MALLOC()/CALLOC(), but I'm also ok with a > TEST_ prefix, my brain can adapt. One of those two flavors has my vote. According to the previous discussion, so which method do we need to use now? If you have a consensus, if necessary, I can continue to do this work. 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. If we add a new macro, whether these old usage should be changed as well.
diff --git a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c index 67a62a5a8895..18afc2000a74 100644 --- a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c +++ 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"); /* The format must be set to 0. 0 for VMX, 1 for SVM. */ set_default_vmx_state(state, state_sz);
There is a 'malloc' call in test_vmx_nested_state function, which can be unsuccessful. This patch will add the malloc failure checking to avoid possible null dereference and give more information about test fail reasons. Signed-off-by: Kunwu Chan <chentao@kylinos.cn> --- tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c | 1 + 1 file changed, 1 insertion(+)