* Re: [PATCH] KVM: selftests: Drop the unsigned >= 0 assertions in test_write/test_read
2026-09-17 18:10 [PATCH] KVM: selftests: Drop the unsigned >= 0 assertions in test_write/test_read Gokul K
@ 2026-09-17 20:28 ` Sean Christopherson
0 siblings, 0 replies; 2+ messages in thread
From: Sean Christopherson @ 2026-09-17 20:28 UTC (permalink / raw)
To: Gokul K; +Cc: Paolo Bonzini, kvm, Shuah Khan, linux-kselftest, linux-kernel
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;
-}
^ permalink raw reply [flat|nested] 2+ messages in thread