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

[Sean Christopherson <seanjc@xxxxxxxxxx>]

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);