tidy some comments

Signed-off-by: Leah Rowe <leah@libreboot.org>

util/nvmutil/include/common.h

@@ -1,6 +1,4 @@
-/*
+/* SPDX-License-Identifier: MIT
- * SPDX-License-Identifier: MIT
- *
  * Copyright (c) 2022-2026 Leah Rowe <leah@libreboot.org>
  */
 
@@ -422,7 +420,7 @@ const char *getnvmprogname(void);
 
 char *new_tmpfile(int *fd, int local, const char *path);
 int mkstemp_n(char *template);
-char *x_c_tmpdir(void);
+char *get_tmpdir(void);
 int close_on_eintr(int fd);
 int fsync_on_eintr(int fd);
 
@@ -487,6 +485,5 @@ typedef char bool_no_loop_eagain[(NO_LOOP_EAGAIN==0)?1:-1];
 typedef char bool_off_err[(OFF_ERR==0)?1:-1];
 typedef char bool_off_reset[(OFF_RESET==0||OFF_RESET==1)?1:-1];
 
-
 #endif
 #endif

util/nvmutil/lib/checksum.c

@@ -1,10 +1,7 @@
 /* SPDX-License-Identifier: MIT
- *
  * Copyright (c) 2022-2026 Leah Rowe <leah@libreboot.org>
  *
  * Functions related to GbE NVM checksums.
- *
- * Related file: word.c
  */
 
 #include <sys/types.h>
@@ -42,15 +39,14 @@ read_checksums(void)
 	if (cmd->arg_part)
 		_max_invalid = 1;
 
-	/*
+	/* Skip verification on this part,
-	 * Skip verification on this part,
 	 * but only when arg_part is set.
 	 */
 	_skip_part = f->part ^ 1;
 
 	for (_p = 0; _p < 2; _p++) {
-		/*
+
-		 * Only verify a part if it was *read*
+		/* Only verify a part if it was *read*
 		 */
 		if (cmd->arg_part && (_p == _skip_part))
 			continue;
@@ -61,9 +57,11 @@ read_checksums(void)
 	}
 
 	if (_num_invalid >= _max_invalid) {
+
 		if (_max_invalid == 1)
 			err(ECANCELED, "%s: part %lu has a bad checksum",
 			    f->fname, (unsigned long)f->part);
+
 		err(ECANCELED, "%s: No valid checksum found in file",
 		    f->fname);
 	}

util/nvmutil/lib/command.c

@@ -1,8 +1,5 @@
 /* SPDX-License-Identifier: MIT
- *
  * Copyright (c) 2022-2026 Leah Rowe <leah@libreboot.org>
- *
- * Command handlers for nvmutil
  */
 
 #include <sys/types.h>
@@ -35,9 +32,6 @@ sanitize_command_list(void)
 		sanitize_command_index(c);
 }
 
-/* TODO: specific config checks per command
- */
-
 void
 sanitize_command_index(unsigned long c)
 {
@@ -142,7 +136,8 @@ set_cmd_args(int argc, char *argv[])
 	if (x->no_cmd)
 		usage();
 
-	/* Maintainer bugs */
+	/* Maintainer bug
+	 */
 	if (cmd->arg_part && argc < 4)
 		err(EINVAL,
 		    "arg_part set for command that needs argc4");
@@ -172,7 +167,8 @@ conv_argv_part_num(const char *part_str)
 	if (part_str[0] == '\0' || part_str[1] != '\0')
 		err(EINVAL, "Partnum string '%s' wrong length", part_str);
 
-	/* char signedness is implementation-defined */
+	/* char signedness is implementation-defined
+	 */
 	ch = (unsigned char)part_str[0];
 	if (ch < '0' || ch > '1')
 		err(EINVAL, "Bad part number (%c)", ch);
@@ -286,16 +282,14 @@ set_mac_nib(unsigned long mac_str_pos,
 		err(EINVAL, "Invalid character '%c'",
 		    mac->str[mac_str_pos + mac_nib_pos]);
 
-	/*
+	/* If random, ensure that local/unicast bits are set.
-	 * If random, ensure that local/unicast bits are set.
 	 */
 	if ((mac_byte_pos == 0) && (mac_nib_pos == 1) &&
 	    ((mac_ch | 0x20) == 'x' ||
 	    (mac_ch == '?')))
 		hex_num = (hex_num & 0xE) | 2; /* local, unicast */
 
-	/*
+	/* MAC words stored big endian in-file, little-endian
-	 * MAC words stored big endian in-file, little-endian
 	 * logically, so we reverse the order.
 	 */
 	mac->mac_buf[mac_byte_pos >> 1] |= hex_num <<

util/nvmutil/lib/file.c

@@ -1,8 +1,5 @@
 /* SPDX-License-Identifier: MIT
- *
  * Copyright (c) 2026 Leah Rowe <leah@libreboot.org>
- *
- * Safe file handling.
  */
 
 #include <sys/types.h>
@@ -51,6 +48,9 @@ err_same_file:
 	return -1;
 }
 
+/* open() but with abort traps
+ */
+
 void
 xopen(int *fd_ptr, const char *path, int flags, struct stat *st)
 {
@@ -67,8 +67,8 @@ xopen(int *fd_ptr, const char *path, int flags, struct stat *st)
 		err(errno, "%s: file not seekable", path);
 }
 
-/* Ensure rename() is durable by syncing the
+/* fsync() the directory of a file,
- * directory containing the target file.
+ * useful for atomic writes
  */
 
 int
@@ -177,28 +177,11 @@ err_fsync_dir:
 	return -1;
 }
 
-/* create new tmpfile path
+/* returns ptr to path (string). if local>0:
+ * make tmpfile in the same directory as the
+ * file. if local==0, use TMPDIR
  *
- * ON SUCCESS:
+ * if local==0, the 3rd argument is ignored
- *
- * returns ptr to path string on success
- * ALSO: the int at *fd will be set,
- *       indicating the file descriptor
- *
- * ON ERROR:
- *
- * return NULL (*fd not touched)
- *
- * malloc() may set errno, but you should
- * not rely on errno from this function
- *
- * local: if non-zero, then only a file
- * name will be given, relative to
- * the current file name. for this,
- * the 3rd argument (path) must be non-null
- *
- * if local is zero, then 3rd arg (path)
- * is irrelevant and can be NULL
  */
 
 char *
@@ -225,16 +208,6 @@ new_tmpfile(int *fd, int local, const char *path)
 	int fd_tmp = -1;
 	int flags;
 
-	/*
-	 * 256 is the most
-	 * conservative path
-	 * size limit (posix),
-	 * but 4096 is modern
-	 *
-	 * set PATH_LEN as you
-	 * wish, at build time
-	 */
-
 #if defined(PATH_LEN) && \
     (PATH_LEN) >= 256
 	maxlen = PATH_LEN;
@@ -265,7 +238,7 @@ new_tmpfile(int *fd, int local, const char *path)
 		 */
 		tmpdir_len = xstrxlen(default_tmpname, maxlen);
 	} else {
-		base = x_c_tmpdir();
+		base = get_tmpdir();
 
 		if (base == NULL)
 			base = tmp_default;
@@ -392,8 +365,12 @@ lock_file(int fd, int flags)
 	return 0;
 }
 
+/* return TMPDIR, or fall back
+ * to portable defaults
+ */
+
 char *
-x_c_tmpdir(void)
+get_tmpdir(void)
 {
 	char *t;
 	struct stat st;
@@ -401,9 +378,12 @@ x_c_tmpdir(void)
 	t = getenv("TMPDIR");
 
 	if (t && *t) {
+
 		if (stat(t, &st) == 0 && S_ISDIR(st.st_mode)) {
+
 			if ((st.st_mode & S_IWOTH) && !(st.st_mode & S_ISVTX))
 				return NULL;
+
 			return t;
 		}
 	}
@@ -434,11 +414,11 @@ mkstemp_n(char *template)
 	    "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
 
 	unsigned long r;
-	unsigned long max_len =
+#if defined(PATH_LEN) && \
-#ifndef PATH_LEN
+    (PATH_LEN) >= 256
-	    4096;
+	unsigned long max_len = PATH_LEN;
 #else
-	    (PATH_LEN);
+	unsigned long max_len = 4096;
 #endif
 
 	len = xstrxlen(template, max_len);
@@ -579,45 +559,32 @@ err_rw_file_exact:
 	return -1;
 }
 
-/* prw() - portable read-write
+/* prw() - portable read-write with more
+ * safety checks than barebones libc
  *
- * This implements a portable analog of pwrite()
+ * portable pwrite/pread on request, or real
- * and pread() - note that this version is not
+ * pwrite/pread libc functions can be used.
- * thread-safe (race conditions are possible on
+ * the portable (non-libc) pread/pwrite is not
- * shared file descriptors).
+ * thread-safe, because it does not prevent or
- *
+ * mitigate race conditions on file descriptors
- * This limitation is acceptable, since nvmutil is
- * single-threaded. Portability is the main goal.
  *
  * If you need real pwrite/pread, just compile
  * with flag: HAVE_REAL_PREAD_PWRITE=1
  *
  * A fallback is provided for regular read/write.
- * rw_type can be IO_READ, IO_WRITE, IO_PREAD
+ * rw_type can be IO_READ (read), IO_WRITE (write),
- * or IO_PWRITE
+ * IO_PREAD (pread) or IO_PWRITE
  *
  * loop_eagain does a retry loop on EAGAIN if set
  * loop_eintr does a retry loop on EINTR if set
  *
- * Unlike the bare syscalls, prw() does security
+ * race conditions on non-libc pread/pwrite:
- * checks e.g. checks NULL strings, checks bounds,
+ * if a file offset changes, abort, to mitage.
- * also mitigates a few theoretical libc bugs.
- * It is designed for extremely safe single-threaded
- * I/O on applications that need it.
  *
- * NOTE: If you use loop_eagain (1), you enable wait
+ * off_reset 1: reset the file offset *once* if
- * loop on EAGAIN. Beware if using this on a non-blocking
+ *              a change was detected, assuming
- * pipe (it could spin indefinitely).
+ *              nothing else is touching it now
- *
+ * off_reset 0: never reset if changed
- * off_reset: if zero, and using fallback pwrite/pread
- *            analogs, we check if a file offset changed,
- *            which would indicate another thread changed
- *            it, and return error, without resetting the
- *            file - this would allow that thread to keep
- *            running, but we could then cause a whole
- *            program exit if we wanted to.
- *            if not zero:
- *            we reset and continue, and pray for the worst.
  */
 
 long
@@ -644,16 +611,15 @@ prw(int fd, void *mem, unsigned long nrw,
 
 	r = -1;
 
-	/* Programs like cat can use this,
+	/* do not use loop_eagain on
-	   so we only check if it's a normal
+	 * normal files
-	   file if not looping EAGAIN */
+	 */
+
 	if (!loop_eagain) {
-		/*
+		/* check whether the file
-		 * Checking on every run of prw()
+		 * changed
-		 * is expensive if called many
-		 * times, but is defensive in
-		 * case the status changes.
 		 */
+
 		if (check_file(fd, &st) == -1)
 			return -1;
 	}
@@ -703,39 +669,21 @@ real_pread_pwrite:
 		verified = lseek_on_eintr(fd, (off_t)0, SEEK_CUR,
 		    loop_eagain, loop_eintr);
 
-		/*
+		/* abort if the offset changed,
-		 * Partial thread-safety: detect
+		 * indicating race condition. if
-		 * if the offset changed to what
+		 * off_reset enabled, reset *ONCE*
-		 * we previously got. If it did,
-		 * then another thread may have
-		 * changed it. Enabled if
-		 * off_reset is OFF_RESET.
-		 *
-		 * We do this *once*, on the theory
-		 * that nothing is touching it now.
 		 */
+
 		if (off_reset && off != verified)
 			lseek_on_eintr(fd, off, SEEK_SET,
 			    loop_eagain, loop_eintr);
 
 		do {
-			/*
+			/* check offset again, repeatedly.
-			 * Verify again before I/O
+			 * even if off_reset is set, this
-			 * (even with OFF_ERR)
+			 * aborts if offsets change again
-			 *
-			 * This implements the first check
-			 * even with OFF_ERR, but without
-			 * the recovery. On ERR_RESET, if
-			 * the check fails again, then we
-			 * know something else is touching
-			 * the file, so it's best that we
-			 * probably leave it alone and err.
-			 *
-			 * In other words, ERR_RESET only
-			 * tolerates one change. Any more
-			 * will cause an exit, including
-			 * per EINTR/EAGAIN re-spin.
 			 */
+
 			verified = lseek_on_eintr(fd, (off_t)0, SEEK_CUR,
 			    loop_eagain, loop_eintr);
 
@@ -831,9 +779,7 @@ err_is_file:
 	return -1;
 }
 
-/* Check weirdness on buggy libc.
+/* POSIX can say whatever it wants.
- *
- * POSIX can say whatever it wants.
  * specification != implementation
  */
 
@@ -888,11 +834,6 @@ err_rw_over_nrw:
 
 #if !defined(HAVE_REAL_PREAD_PWRITE) || \
     HAVE_REAL_PREAD_PWRITE < 1
-/*
- * lseek_on_eintr() does lseek() but optionally
- * on an EINTR/EAGAIN wait loop. Used by prw()
- * for setting offsets for positional I/O.
- */
 off_t
 lseek_on_eintr(int fd, off_t off, int whence,
     int loop_eagain, int loop_eintr)

util/nvmutil/lib/io.c

@@ -1,5 +1,4 @@
 /* SPDX-License-Identifier: MIT
- *
  * Copyright (c) 2026 Leah Rowe <leah@libreboot.org>
  *
  * I/O functions specific to nvmutil.
@@ -44,12 +43,11 @@ open_gbe_file(void)
 	if (_flags == -1)
 		err(errno, "%s: fcntl(F_GETFL)", f->fname);
 
-	/*
+	/* O_APPEND allows POSIX write() to ignore
-	 * O_APPEND must not be used, because this
+	 * the current write offset and write at EOF,
-	 * allows POSIX write() to ignore the
+	 * which would break positional read/write
-	 * current write offset and write at EOF,
-	 * which would therefore break pread/pwrite
 	 */
+
 	if (_flags & O_APPEND)
 		err(EIO, "%s: O_APPEND flag", f->fname);
 
@@ -223,10 +221,10 @@ write_to_gbe_bin(void)
 
 	write_gbe_file();
 
-	/*
+	/* We may otherwise read from
-	 * We may otherwise read from
 	 * cache, so we must sync.
 	 */
+
 	if (fsync_on_eintr(f->tmp_fd) == -1)
 		err(errno, "%s: fsync (pre-verification)",
 		    f->tname);
@@ -239,11 +237,6 @@ write_to_gbe_bin(void)
 	if (f->io_err_gbe)
 		err(EIO, "%s: bad write", f->fname);
 
-	/*
-	 * success!
-	 * now just rename the tmpfile
-	 */
-
 	saved_errno = errno;
 
 	if (close_on_eintr(f->tmp_fd) == -1) {
@@ -258,6 +251,11 @@ write_to_gbe_bin(void)
 
 	errno = saved_errno;
 
+	/* tmpfile written, now we
+	 * rename it back to the main file
+	 * (we do atomic writes)
+	 */
+
 	f->tmp_fd = -1;
 	f->gbe_fd = -1;
 
@@ -275,6 +273,7 @@ write_to_gbe_bin(void)
 
 			/* removed by rename
 			 */
+
 			if (f->tname != NULL)
 				free(f->tname);
 
@@ -338,17 +337,17 @@ check_written_part(unsigned long p)
 	    f->rw_check_partial_read[p])
 		return;
 
-	/*
+	/* We only load one part on-file, into memory but
-	 * We only load one part on-file, into memory but
 	 * always at offset zero, for post-write checks.
-	 * That's why we hardcode good_checksum(0).
+	 * That's why we hardcode good_checksum(0)
 	 */
+
 	buf_restore = f->buf;
 
-	/*
+	/* good_checksum works on f->buf
-	 * good_checksum works on f->buf
 	 * so let's change f->buf for now
 	 */
+
 	f->buf = f->pad;
 
 	if (good_checksum(0))
@@ -427,7 +426,8 @@ gbe_mv(void)
 	char *dest_tmp;
 	int dest_fd;
 
-	/* will be set 0 if it doesn't */
+	/* will be set 0 if it doesn't
+	 */
 	tmp_gbe_bin_exists = 1;
 
 	dest_tmp = NULL;
@@ -438,8 +438,8 @@ gbe_mv(void)
 	rval = rename(f->tname, f->fname);
 
 	if (rval > -1) {
-		/*
+
-		 * same filesystem
+		/* rename on same filesystem
 		 */
 
 		tmp_gbe_bin_exists = 0;
@@ -455,19 +455,22 @@ gbe_mv(void)
 	if (errno != EXDEV)
 		goto ret_gbe_mv;
 
-	/* cross-filesystem rename */
+	/*
+	 * OR, cross-filesystem rename:
+	 */
 
 	if ((rval = f->tmp_fd = open(f->tname,
 	    O_RDONLY | O_BINARY)) == -1)
 		goto ret_gbe_mv;
 
-	/* create replacement temp in target directory */
+	/* create replacement temp in target directory
+	 */
 	dest_tmp = new_tmpfile(&dest_fd, 1, f->fname);
 	if (dest_tmp == NULL)
 		goto ret_gbe_mv;
 
-	/* copy data */
+	/* copy data
-
+	 */
 	rval = rw_file_exact(f->tmp_fd, f->bufcmp,
 	    f->gbe_file_size, 0, IO_PREAD,
 	    NO_LOOP_EAGAIN, LOOP_EINTR,
@@ -520,8 +523,7 @@ ret_gbe_mv:
 		f->tmp_fd = -1;
 	}
 
-	/*
+	/* before this function is called,
-	 * before this function is called,
 	 * tmp_fd may have been moved
 	 */
 	if (tmp_gbe_bin_exists) {
@@ -532,8 +534,7 @@ ret_gbe_mv:
 	}
 
 	if (rval < 0) {
-		/*
+		/* if nothing set errno,
-		 * if nothing set errno,
 		 * we assume EIO, or we
 		 * use what was set
 		 */
@@ -546,8 +547,7 @@ ret_gbe_mv:
 	return rval;
 }
 
-/*
+/* This one is similar to gbe_file_offset,
- * This one is similar to gbe_file_offset,
  * but used to check Gbe bounds in memory,
  * and it is *also* used during file I/O.
  */
@@ -566,12 +566,9 @@ gbe_mem_offset(unsigned long p, const char *f_op)
 	    (f->buf + (unsigned long)gbe_off);
 }
 
-/*
+/* I/O operations filtered here. These operations must
- * I/O operations filtered here. These operations must
  * only write from the 0th position or the half position
  * within the GbE file, and write 4KB of data.
- *
- * This check is called, to ensure just that.
  */
 off_t
 gbe_file_offset(unsigned long p, const char *f_op)

util/nvmutil/lib/num.c

@@ -1,5 +1,4 @@
 /* SPDX-License-Identifier: MIT
- *
  * Copyright (c) 2026 Leah Rowe <leah@libreboot.org>
  *
  * Numerical functions.
@@ -45,6 +44,7 @@ hextonum(char ch_s)
 
 /* Random numbers
  */
+
 unsigned long
 rlong(void)
 {

util/nvmutil/lib/state.c

@@ -1,9 +1,7 @@
 /* SPDX-License-Identifier: MIT
  * Copyright (c) 2022-2026 Leah Rowe <leah@libreboot.org>
  *
- * This tool lets you modify Intel GbE NVM (Gigabit Ethernet
+ * State machine (singleton) for nvmutil data.
- * Non-Volatile Memory) images, e.g. change the MAC address.
- * These images configure your Intel Gigabit Ethernet adapter.
  */
 
 #ifdef __OpenBSD__
@@ -24,12 +22,6 @@
 
 #include "../include/common.h"
 
-/*
- * Initialise program state,
- * load GbE file and verify
- * data, ready for operation
- * (singleton design)
- */
 struct xstate *
 xstatus(int argc, char *argv[])
 {

util/nvmutil/lib/string.c

@@ -1,7 +1,7 @@
 /* SPDX-License-Identifier: MIT
  * Copyright (c) 2026 Leah Rowe <leah@libreboot.org>
  *
- * String handling.
+ * String functions
  */
 
 #include <sys/types.h>
@@ -13,10 +13,10 @@
 
 #include "../include/common.h"
 
-/*
+/* Portable strncmp() that blocks
- * Portable strcmp() but blocks NULL/empty/unterminated
+ * NULL/empty/unterminated strings
- * strings. Even stricter than strncmp().
  */
+
 int
 xstrxcmp(const char *a, const char *b, unsigned long maxlen)
 {
@@ -43,24 +43,16 @@ xstrxcmp(const char *a, const char *b, unsigned long maxlen)
 			return ac - bc;
 	}
 
-	/*
-	 * We reached maxlen, so assume unterminated string.
-	 */
 	err(EINVAL, "Unterminated string in xstrxcmp");
 
-	/*
-	 * Should never reach here. This keeps compilers happy.
-	 */
 	errno = EINVAL;
 	return -1;
 }
 
-/*
+/* Portable strncmp() that blocks
- * strnlen() but aborts on NULL input, and empty strings.
+ * NULL/empty/unterminated strings
- * Our version also prohibits unterminated strings.
- * strnlen() was standardized in POSIX.1-2008 and is not
- * available on some older systems, so we provide our own.
  */
+
 unsigned long
 xstrxlen(const char *scmp, unsigned long maxlen)
 {

util/nvmutil/lib/word.c

@@ -1,9 +1,8 @@
 /* SPDX-License-Identifier: MIT
- *
  * Copyright (c) 2022-2026 Leah Rowe <leah@libreboot.org>
  *
- * Manipulate sixteen-bit little-endian
+ * Manipulate Intel GbE NVM words, which are 16-bit little
- * words on Intel GbE NVM configurations.
+ * endian in the files (MAC address words are big endian).
  */
 
 #include <sys/types.h>
@@ -13,13 +12,6 @@
 
 #include "../include/common.h"
 
-/*
- * GbE NVM files store 16-bit (2-byte) little-endian words.
- * We must therefore swap the order when reading or writing.
- *
- * NOTE: The MAC address words are stored big-endian in-file.
- */
-
 unsigned short
 nvm_word(unsigned long pos16, unsigned long p)
 {

util/nvmutil/nvmutil.c

@@ -1,5 +1,4 @@
 /* SPDX-License-Identifier: MIT
- *
  * Copyright (c) 2022-2026 Leah Rowe <leah@libreboot.org>
  *
  * This tool lets you modify Intel GbE NVM (Gigabit Ethernet