Re: [PATCH] KVM: selftests: Drop the unsigned >= 0 assertions in test_write/test_read

[Sean Christopherson <seanjc@google.com>]

On Thu, Sep 17, 2026, Gokul K wrote:
> test_write() and test_read() both open with
> 
> 	TEST_ASSERT(count >= 0, "Unexpected count, count: %li", count);
> 
> but @count is a size_t, so the condition is always true and the assertion
> can never fire. Building the selftests with -Wextra says so:
> 
>   lib/io.c:51:27: warning: comparison of unsigned expression in '>= 0'
>                   is always true [-Wtype-limits]
>   lib/io.c:128:27: warning: comparison of unsigned expression in '>= 0'
>                   is always true [-Wtype-limits]
> 
> @count has been a size_t since these helpers were added in commit
> 6089ae0bd5e1 ("kvm: selftests: add sync_regs_test"), so this has never
> guarded anything; nothing regressed and there is no behavioural change.
> 
> Note also that the message the assertion would have printed is wrong: %li
> takes a long, not a size_t. That has gone unnoticed precisely because the
> assertion is unreachable, which is a fair summary of the value it adds.
> 
> Delete both. The comment above each one is about a count of zero being
> legitimate, which remains true and is worth keeping.

I disagree.  This code is all rather non-sensical.  Keeping the comment with code
that doesn't handle it correct can't work.  Because per the manpage:

  a read() with a count of 0 returns zero and has no other effects.

which means passing in a size of 0 will hit the "case 0" and fail:

		rc = read(fd, ptr, num_left);

		switch (rc) {
		case -1:
			TEST_ASSERT(errno == EAGAIN || errno == EINTR,
				    "Unexpected read failure,\n"
				    "  rc: %zi errno: %i", rc, errno);
			break;

		case 0:
			TEST_FAIL("Unexpected EOF,\n"  <==========================
				  "   rc: %zi num_read: %zi num_left: %zu",
				  rc, num_read, num_left);
			break;

		default:
			TEST_ASSERT(rc > 0, "Unexpected ret from read,\n"
				    "  rc: %zi errno: %i", rc, errno);
			num_read += rc;
			num_left -= rc;
			ptr += rc;
			break;

The only user of test_read() is tools/testing/selftests/kvm/lib/elf.c, and all
users guarantee a non-zero count.  test_write() is dead code, and test_seq_read()
has a defunct declaration.

The names are also confusing, because it's easy to read it as "test the read()
sycall", not "do read() for this test".

So, I think we should move test_read() into elf.c, drop the absurdly verbose and
unhelpful commentry, then kill off io.c, test_write() and the stale test_seq_read().

diff --git tools/testing/selftests/kvm/Makefile.kvm tools/testing/selftests/kvm/Makefile.kvm
index 6a1482e3a286..610b4d959673 100644
--- tools/testing/selftests/kvm/Makefile.kvm
+++ tools/testing/selftests/kvm/Makefile.kvm
@@ -6,7 +6,6 @@ all:
 LIBKVM += lib/assert.c
 LIBKVM += lib/elf.c
 LIBKVM += lib/guest_modes.c
-LIBKVM += lib/io.c
 LIBKVM += lib/kvm_util.c
 LIBKVM += lib/lru_gen_util.c
 LIBKVM += lib/memstress.c
diff --git tools/testing/selftests/kvm/include/test_util.h tools/testing/selftests/kvm/include/test_util.h
index a6a3e1657895..e558346c3b69 100644
--- tools/testing/selftests/kvm/include/test_util.h
+++ tools/testing/selftests/kvm/include/test_util.h
@@ -49,10 +49,6 @@ do {								\
 
 #define TEST_REQUIRE(f) __TEST_REQUIRE(f, "Requirement not met: %s", #f)
 
-ssize_t test_write(int fd, const void *buf, size_t count);
-ssize_t test_read(int fd, void *buf, size_t count);
-int test_seq_read(const char *path, char **bufp, size_t *sizep);
-
 void __printf(5, 6) test_assert(bool exp, const char *exp_str,
 				const char *file, unsigned int line,
 				const char *fmt, ...);
diff --git tools/testing/selftests/kvm/lib/elf.c tools/testing/selftests/kvm/lib/elf.c
index 1924a9895834..d996e289f4af 100644
--- tools/testing/selftests/kvm/lib/elf.c
+++ tools/testing/selftests/kvm/lib/elf.c
@@ -12,6 +12,44 @@
 
 #include "kvm_util.h"
 
+static ssize_t elf_read(int fd, void *buf, size_t count)
+{
+	ssize_t rc;
+	ssize_t num_read = 0;
+	size_t num_left = count;
+	char *ptr = buf;
+
+	TEST_ASSERT(count, "Count must be non-zero");
+
+	do {
+		rc = read(fd, ptr, num_left);
+
+		switch (rc) {
+		case -1:
+			TEST_ASSERT(errno == EAGAIN || errno == EINTR,
+				    "Unexpected read failure,\n"
+				    "  rc: %zi errno: %i", rc, errno);
+			break;
+
+		case 0:
+			TEST_FAIL("Unexpected EOF,\n"
+				  "   rc: %zi num_read: %zi num_left: %zu",
+				  rc, num_read, num_left);
+			break;
+
+		default:
+			TEST_ASSERT(rc > 0, "Unexpected ret from read,\n"
+				    "  rc: %zi errno: %i", rc, errno);
+			num_read += rc;
+			num_left -= rc;
+			ptr += rc;
+			break;
+		}
+	} while (num_read < count);
+
+	return num_read;
+}
+
 static void elfhdr_get(const char *filename, Elf64_Ehdr *hdrp)
 {
 	off_t offset_rv;
@@ -31,7 +69,7 @@ static void elfhdr_get(const char *filename, Elf64_Ehdr *hdrp)
 	 * the real size of the ELF header.
 	 */
 	unsigned char ident[EI_NIDENT];
-	test_read(fd, ident, sizeof(ident));
+	elf_read(fd, ident, sizeof(ident));
 	TEST_ASSERT((ident[EI_MAG0] == ELFMAG0) && (ident[EI_MAG1] == ELFMAG1)
 		&& (ident[EI_MAG2] == ELFMAG2) && (ident[EI_MAG3] == ELFMAG3),
 		"ELF MAGIC Mismatch,\n"
@@ -79,7 +117,7 @@ static void elfhdr_get(const char *filename, Elf64_Ehdr *hdrp)
 	offset_rv = lseek(fd, 0, SEEK_SET);
 	TEST_ASSERT(offset_rv == 0, "Seek to ELF header failed,\n"
 		"  rv: %zi expected: %i", offset_rv, 0);
-	test_read(fd, hdrp, sizeof(*hdrp));
+	elf_read(fd, hdrp, sizeof(*hdrp));
 	TEST_ASSERT(hdrp->e_phentsize == sizeof(Elf64_Phdr),
 		"Unexpected physical header size,\n"
 		"  hdrp->e_phentsize: %x\n"
@@ -146,7 +184,7 @@ void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename)
 
 		/* Read in the program header. */
 		Elf64_Phdr phdr;
-		test_read(fd, &phdr, sizeof(phdr));
+		elf_read(fd, &phdr, sizeof(phdr));
 
 		/* Skip if this header doesn't describe a loadable segment. */
 		if (phdr.p_type != PT_LOAD)
@@ -186,7 +224,7 @@ void kvm_vm_elf_load(struct kvm_vm *vm, const char *filename)
 				"  expected: 0x%jx",
 				n1, errno, (intmax_t) offset_rv,
 				(intmax_t) phdr.p_offset);
-			test_read(fd, addr_gva2hva(vm, phdr.p_vaddr),
+			elf_read(fd, addr_gva2hva(vm, phdr.p_vaddr),
 				phdr.p_filesz);
 		}
 	}
diff --git tools/testing/selftests/kvm/lib/io.c tools/testing/selftests/kvm/lib/io.c
deleted file mode 100644
index fedb2a741f0b..000000000000
--- tools/testing/selftests/kvm/lib/io.c
+++ /dev/null
@@ -1,157 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * tools/testing/selftests/kvm/lib/io.c
- *
- * Copyright (C) 2018, Google LLC.
- */
-
-#include "test_util.h"
-
-/* Test Write
- *
- * A wrapper for write(2), that automatically handles the following
- * special conditions:
- *
- *   + Interrupted system call (EINTR)
- *   + Write of less than requested amount
- *   + Non-block return (EAGAIN)
- *
- * For each of the above, an additional write is performed to automatically
- * continue writing the requested data.
- * There are also many cases where write(2) can return an unexpected
- * error (e.g. EIO).  Such errors cause a TEST_ASSERT failure.
- *
- * Note, for function signature compatibility with write(2), this function
- * returns the number of bytes written, but that value will always be equal
- * to the number of requested bytes.  All other conditions in this and
- * future enhancements to this function either automatically issue another
- * write(2) or cause a TEST_ASSERT failure.
- *
- * Args:
- *  fd    - Opened file descriptor to file to be written.
- *  count - Number of bytes to write.
- *
- * Output:
- *  buf   - Starting address of data to be written.
- *
- * Return:
- *  On success, number of bytes written.
- *  On failure, a TEST_ASSERT failure is caused.
- */
-ssize_t test_write(int fd, const void *buf, size_t count)
-{
-	ssize_t rc;
-	ssize_t num_written = 0;
-	size_t num_left = count;
-	const char *ptr = buf;
-
-	/* Note: Count of zero is allowed (see "RETURN VALUE" portion of
-	 * write(2) manpage for details.
-	 */
-	TEST_ASSERT(count >= 0, "Unexpected count, count: %li", count);
-
-	do {
-		rc = write(fd, ptr, num_left);
-
-		switch (rc) {
-		case -1:
-			TEST_ASSERT(errno == EAGAIN || errno == EINTR,
-				    "Unexpected write failure,\n"
-				    "  rc: %zi errno: %i", rc, errno);
-			continue;
-
-		case 0:
-			TEST_FAIL("Unexpected EOF,\n"
-				  "  rc: %zi num_written: %zi num_left: %zu",
-				  rc, num_written, num_left);
-			break;
-
-		default:
-			TEST_ASSERT(rc >= 0, "Unexpected ret from write,\n"
-				"  rc: %zi errno: %i", rc, errno);
-			num_written += rc;
-			num_left -= rc;
-			ptr += rc;
-			break;
-		}
-	} while (num_written < count);
-
-	return num_written;
-}
-
-/* Test Read
- *
- * A wrapper for read(2), that automatically handles the following
- * special conditions:
- *
- *   + Interrupted system call (EINTR)
- *   + Read of less than requested amount
- *   + Non-block return (EAGAIN)
- *
- * For each of the above, an additional read is performed to automatically
- * continue reading the requested data.
- * There are also many cases where read(2) can return an unexpected
- * error (e.g. EIO).  Such errors cause a TEST_ASSERT failure.  Note,
- * it is expected that the file opened by fd at the current file position
- * contains at least the number of requested bytes to be read.  A TEST_ASSERT
- * failure is produced if an End-Of-File condition occurs, before all the
- * data is read.  It is the callers responsibility to assure that sufficient
- * data exists.
- *
- * Note, for function signature compatibility with read(2), this function
- * returns the number of bytes read, but that value will always be equal
- * to the number of requested bytes.  All other conditions in this and
- * future enhancements to this function either automatically issue another
- * read(2) or cause a TEST_ASSERT failure.
- *
- * Args:
- *  fd    - Opened file descriptor to file to be read.
- *  count - Number of bytes to read.
- *
- * Output:
- *  buf   - Starting address of where to write the bytes read.
- *
- * Return:
- *  On success, number of bytes read.
- *  On failure, a TEST_ASSERT failure is caused.
- */
-ssize_t test_read(int fd, void *buf, size_t count)
-{
-	ssize_t rc;
-	ssize_t num_read = 0;
-	size_t num_left = count;
-	char *ptr = buf;
-
-	/* Note: Count of zero is allowed (see "If count is zero" portion of
-	 * read(2) manpage for details.
-	 */
-	TEST_ASSERT(count >= 0, "Unexpected count, count: %li", count);
-
-	do {
-		rc = read(fd, ptr, num_left);
-
-		switch (rc) {
-		case -1:
-			TEST_ASSERT(errno == EAGAIN || errno == EINTR,
-				    "Unexpected read failure,\n"
-				    "  rc: %zi errno: %i", rc, errno);
-			break;
-
-		case 0:
-			TEST_FAIL("Unexpected EOF,\n"
-				  "   rc: %zi num_read: %zi num_left: %zu",
-				  rc, num_read, num_left);
-			break;
-
-		default:
-			TEST_ASSERT(rc > 0, "Unexpected ret from read,\n"
-				    "  rc: %zi errno: %i", rc, errno);
-			num_read += rc;
-			num_left -= rc;
-			ptr += rc;
-			break;
-		}
-	} while (num_read < count);
-
-	return num_read;
-}