On Thu, Jul 21, 2022, Aaron Lewis wrote:
> > > --- a/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
> > > +++ b/tools/testing/selftests/kvm/x86_64/userspace_msr_exit_test.c
> > > @@ -734,6 +734,99 @@ static void test_msr_permission_bitmap(void)
> > > kvm_vm_free(vm);
> > > }
> > >
> > > +static void test_results(int rc, const char *scmd, bool expected_success)
> >
> > Rather than pass in "success expected", pass in the actual value and the valid
> > mask. Then you can spit out the problematic value in the assert and be kind to
> > future debuggers.
> >
> > And similarly, make the __vm_ioctl() call here instead of in the "caller" and name
> > this __test_ioctl() (rename as necessary, see below) to show it's relationship with
> > the macro.
>
> The other comments look good. I'll update.
>
> This one is a bit tricky though. I did originally have __vm_ioctl()
> in test_results() (or whatever name it will end up with), but the
> static assert in kvm_do_ioctl() gave me problems. Unless I make
> test_results() a macro, I have to force cmd to a uint64_t or something
> other than a literal, then I get this:
>
> include/kvm_util_base.h:190:39: error: expression in static assertion
> is not constant
> 190 | static_assert(!_IOC_SIZE(cmd) || sizeof(*arg) ==
> _IOC_SIZE(cmd), ""); \
> | ^
> include/kvm_util_base.h:213:9: note: in expansion of macro ‘kvm_do_ioctl’
> 213 | kvm_do_ioctl((vm)->fd, cmd, arg); \
>
> That's not the only problem. In order to pass 'arg' in I would have
> to pass it as a void *, making sizeof(*arg) wrong.
>
> Being that the ioctl call was the first thing I did in that function I
> opted to make it a part of test_ioctl() rather than making
> test_results() a macro.
>
> If only C had templates :)
Eh, what C++ can do with templates, C can usually do with macros :-)
Sans backslashes, I think this can simply be:
#define test_user_exit_msr_ioctl(vm, cmd, arg, val, valid_mask)
({
int r = __vm_ioctl(vm, cmd, arg);
if (val & valid_mask)
TEST_ASSERT(!r, KVM_IOCTL_ERROR(cmd, r));
else
TEST_ASSERT(r == -1 && errno == EINVAL,
"Wanted EINVAL with val = 0x%llx, got rc: %i errno: %i (%s)",
val, r, errno, strerror(errno))
})
It doesn't print "val" when success is expected, but I'm ok with that. Though I
suspect that adding a common macro to print additional info on an unexpected
ioctl() result would be useful for other tests.
