Re: [PATCH v7 05/11] KVM: selftests: Read binary stat data in lib

Sean Christopherson
2022-05-05T18:44:10Z

On Thu, May 05, 2022, Sean Christopherson wrote:
> On Tue, May 03, 2022, Ben Gardon wrote:
> Eww. I really, really hate code that asserts on a value and then returns that
> same value. E.g. looking at just the declaration of read_stat_data() and the
> change in stats_test(), I genuinely thought this patch dropped the assert. The
> assert in vm_get_stat() also added to the confusion (I was reviewing that patch,
> not this one).
>
> Rather than return the number of entries read, just assert that the number of
> elements to be read is non-zero, then vm_get_stat() doesn't need to assert because
> it'll be impossible to read anything but one entry without asserting.

Ah, and __vm_get_stat() can do:

for (i = 0; i < vm->stats_header.num_desc; ++i) {
desc = get_stats_descriptor(vm->stats_desc, i, &vm->stats_header);

if (strcmp(desc->name, stat_name))
continue;

read_stat_data(vm->stats_fd, &vm->stats_header, desc, data,
max_elements);
return;
}

TEST_FAIL("Stat '%s' does not exist\n", stat_name);

>
> void read_stat_data(int stats_fd, struct kvm_stats_header *header,
> struct kvm_stats_desc *desc, uint64_t *data,
> size_t max_elements)
> {
> size_t nr_elements = min_t(size_t, desc->size, max_elements);
> size_t size = nr_elements * sizeof(*data);
> ssize_t ret;
>
> TEST_ASSERT(size, "No elements in stat '%s'", desc->name);
>
> ret = pread(stats_fd, data, size, header->data_offset + desc->offset);
>
> TEST_ASSERT(ret == size,
> "pread() failed on stat '%s', wanted %lu bytes, got %ld",
> desc->name, size, ret);

Related to not printing a raw EINVAL (similar to above), it might be worth special
casing the errno path, e.g.

TEST_ASSERT(ret >= 0, "pread() failed on stat '%s', errno: %i (%s)",
desc->name, errno, strerror(errno));
TEST_ASSERT(ret == size,
"pread() on stat '%s' read %ld bytes, wanted %lu bytes",
desc->name, size, ret);