From 51b5020b8eb2a8f231b0ed5b86f554dc42f7eb32 Mon Sep 17 00:00:00 2001 From: Andrea Mazzoleni Date: Sun, 20 Sep 2026 15:56:14 +0200 Subject: [PATCH] Exit code and logging invariants for log_fatal and hardware log_error --- AGENTS.md | 3 +++ cmdline/app.h | 2 +- cmdline/check.c | 5 +++++ cmdline/dry.c | 5 +++++ cmdline/parity.c | 19 +++++++++++++++---- cmdline/scrub.c | 5 +++++ cmdline/snapraid.c | 12 ++++++++++-- cmdline/state.c | 14 +++++++------- cmdline/state.h | 2 +- cmdline/support.c | 40 +++++++++++++++++++++++++++++++++++++++- cmdline/support.h | 5 +++++ cmdline/sync.c | 17 +++++++++++------ cmdline/touch.c | 13 ++++++++++++- cmdline/unixapp.c | 2 +- 14 files changed, 120 insertions(+), 24 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 525141ce..2dfbdcbf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -124,6 +124,9 @@ SnapRAID is designed to recover from an interruption, including `SIGKILL`, at an Or use callback iteration helpers: `tommy_list_foreach()`, `tommy_list_foreach_arg()`, `tommy_hashdyn_foreach()`, etc. - **Cleanup**: Use `goto` pattern for cleanup in complex functions when appropriate - **Fatal paths**: Use `exit(EXIT_FAILURE)` for unrecoverable errors; `os_abort()` for internal inconsistencies +- **Exit Code & Logging Invariants**: + - `log_fatal()`: Any invocation of `log_fatal()` represents an unrecoverable failure and must force a non-zero exit code upon termination. + - `log_error()`: Whenever `log_error()` is called with a hardware error (`is_hw_errno(err)`), it must force a non-zero exit code upon termination. #### Threading diff --git a/cmdline/app.h b/cmdline/app.h index 01a1bc7e..def91c30 100644 --- a/cmdline/app.h +++ b/cmdline/app.h @@ -13,8 +13,8 @@ #define STATIC_ASSERT(name, cond) typedef char static_assert_ ## name[(cond) ? 1 : -1] +#define EDATA (-1) /**< Silent data corruption. */ #define EINTERNAL EFAULT /**< Internal assertion failed. */ -#define EDATA EIO /**< Silent data corruption. */ #define ESOFT EINVAL /**< Software error, like permission denied. */ #define EUSER EINVAL /**< Invalid value specified by the user. */ #define EEXTERNAL EINVAL /**< Invalid external interface behaviour. */ diff --git a/cmdline/check.c b/cmdline/check.c index 1c0f8c84..607ec2d8 100644 --- a/cmdline/check.c +++ b/cmdline/check.c @@ -3230,6 +3230,11 @@ int state_check(struct snapraid_state* state, int fix, block_off_t blockstart, b if (process_error != 0) return -1; + if (log_hardware_errors() != 0) { + msg_status("Everything done, but hardware errors were encountered in the process\n"); + return -1; + } + msg_status("Everything OK\n"); return 0; diff --git a/cmdline/dry.c b/cmdline/dry.c index 0f415259..03066564 100644 --- a/cmdline/dry.c +++ b/cmdline/dry.c @@ -491,6 +491,11 @@ int state_dry(struct snapraid_state* state, block_off_t blockstart, block_off_t if (process_error != 0) return -1; + if (log_hardware_errors() != 0) { + msg_status("Everything done, but hardware errors were encountered in the process\n"); + return -1; + } + msg_status("Everything OK\n"); return 0; diff --git a/cmdline/parity.c b/cmdline/parity.c index 89bb34ec..34bc3a57 100644 --- a/cmdline/parity.c +++ b/cmdline/parity.c @@ -430,8 +430,14 @@ static int parity_handle_fill(struct snapraid_split_handle* split, data_off_t si /* note that in Windows ftruncate is really allocating space */ if (ftruncate(spaceholder_f, WINDOWS_SPACEHOLDER_SIZE) != 0) { - log_fatal(errno, "WARNING Failed to resize the space holder file '%s' to %u bytes.\n", spaceholder_path, WINDOWS_SPACEHOLDER_SIZE); - log_fatal(errno, "Assuming that no more space is available.\n"); + if (is_hw(errno)) { + log_fatal(errno, "Hardware error while resizing space holder file '%s'. %s.\n", spaceholder_path, strerror(errno)); + close(spaceholder_f); + remove(spaceholder_path); + return -1; + } + log_info(0, "WARNING Failed to resize the space holder file '%s' to %u bytes. %s.\n", spaceholder_path, WINDOWS_SPACEHOLDER_SIZE, strerror(errno)); + log_info(0, "Assuming that no more space is available.\n"); close(spaceholder_f); remove(spaceholder_path); return 0; @@ -505,8 +511,13 @@ static int parity_handle_fill(struct snapraid_split_handle* split, data_off_t si #ifdef _WIN32 /* now delete the spaceholder file */ if (remove(spaceholder_path) != 0) { - log_error(errno, "WARNING Failed to remove the space holder file '%s'.\n", spaceholder_path); - log_error(errno, "Continuing anyway.\n"); + int err = errno; + + log_error(err, "WARNING Failed to remove the space holder file '%s'. %s.\n", spaceholder_path, strerror(err)); + if (is_hw(err)) { + return -1; + } + log_error(err, "Continuing anyway.\n"); } #endif diff --git a/cmdline/scrub.c b/cmdline/scrub.c index e2ec6a1a..f5d23474 100644 --- a/cmdline/scrub.c +++ b/cmdline/scrub.c @@ -1264,6 +1264,11 @@ int state_scrub(struct snapraid_state* state, int plan100, int olderthan) if (process_error != 0) return -1; + if (log_hardware_errors() != 0) { + msg_status("Everything done, but hardware errors were encountered in the process\n"); + return -1; + } + msg_status("Everything OK\n"); return 0; diff --git a/cmdline/snapraid.c b/cmdline/snapraid.c index 69971683..ea39f50a 100644 --- a/cmdline/snapraid.c +++ b/cmdline/snapraid.c @@ -1503,11 +1503,15 @@ int snapraid_main(int argc, char* argv[]) state_locate_mark_tail_blocks_for_resync(&state, opt.parity_tail); if (opt.gui_touch_before) { - state_touch(&state); + ret = state_touch(&state); /* save the new state if required */ if (state.need_write) state_write(&state); + + if (ret != 0) { + exit(EXIT_FAILURE); + } } ret = state_snapshot_scan(&state); @@ -1652,7 +1656,7 @@ int snapraid_main(int argc, char* argv[]) memory(); - state_touch(&state); + ret = state_touch(&state); /* * Intercept signals while operating @@ -1779,6 +1783,10 @@ int snapraid_main(int argc, char* argv[]) /* LCOV_EXCL_STOP */ } + /* force failure if hardware errors were encountered */ + if (ret == 0 && log_hardware_errors() != 0) + ret = -1; + /* close log file */ log_close(log_file); diff --git a/cmdline/state.c b/cmdline/state.c index d334ec85..a5e56158 100644 --- a/cmdline/state.c +++ b/cmdline/state.c @@ -353,7 +353,7 @@ static void state_config_check(struct snapraid_state* state, const char* path, t * because we want to use these disks */ if (!state->opt.no_warnings) - log_fatal(EUSER, "DANGER! Ignoring that disks '%s' and '%s' are on the same device\n", disk->name, other->name); + log_error(EUSER, "DANGER! Ignoring that disks '%s' and '%s' are on the same device\n", disk->name, other->name); } else { /* LCOV_EXCL_START */ log_fatal(ESOFT, "Disks '%s' and '%s' are on the same device.\n", disk->mount_point, other->mount_point); @@ -383,7 +383,7 @@ static void state_config_check(struct snapraid_state* state, const char* path, t * because we want to use these disks */ if (!state->opt.no_warnings) - log_fatal(EUSER, "DANGER! Ignoring that disks '%s' and %s '%s' are on the same device\n", disk->mount_point, lev_name(l), state->parity[l].split_map[s].path); + log_error(EUSER, "DANGER! Ignoring that disks '%s' and %s '%s' are on the same device\n", disk->mount_point, lev_name(l), state->parity[l].split_map[s].path); } else { /* LCOV_EXCL_START */ log_fatal(ESOFT, "Disk '%s' and %s '%s' are on the same device.\n", disk->mount_point, lev_name(l), state->parity[l].split_map[s].path); @@ -446,7 +446,7 @@ static void state_config_check(struct snapraid_state* state, const char* path, t * because we want to use these disks */ if (!state->opt.no_warnings) - log_fatal(EUSER, "DANGER! Skipping parities '%s' and '%s' on the same device\n", lev_config_name(l), lev_config_name(j)); + log_error(EUSER, "DANGER! Skipping parities '%s' and '%s' on the same device\n", lev_config_name(l), lev_config_name(j)); } else { /* LCOV_EXCL_START */ log_fatal(ESOFT, "Parity '%s' and '%s' are on the same device.\n", state->parity[l].split_map[s].path, state->parity[j].split_map[t].path); @@ -875,7 +875,7 @@ void state_config(struct snapraid_state* state, const char* path, const char* co dev = 0; *uuid = 0; skip_access = 1; - log_fatal(errno, "DANGER! Skipping inaccessible parity disk '%s'...\n", tag); + log_error(errno, "DANGER! Skipping inaccessible parity disk '%s'...\n", tag); } else { /* LCOV_EXCL_START */ log_fatal(errno, "Error accessing 'parity' dir '%s' specification in '%s' at line %u\n", device, path, line); @@ -1019,7 +1019,7 @@ void state_config(struct snapraid_state* state, const char* path, const char* co if (state->opt.skip_content_access) { /* use a fake device */ dev = 0; - log_fatal(errno, "WARNING! Skipping inaccessible content file '%s'...\n", buffer); + log_error(errno, "WARNING! Skipping inaccessible content file '%s'...\n", buffer); } else { /* LCOV_EXCL_START */ log_fatal(errno, "Error accessing 'content' dir '%s' specification in '%s' at line %u\n", device, path, line); @@ -1139,7 +1139,7 @@ void state_config(struct snapraid_state* state, const char* path, const char* co dev = 0; *uuid = 0; skip_access = 1; - log_fatal(errno, "DANGER! Skipping inaccessible data disk '%s'...\n", buffer); + log_error(errno, "DANGER! Skipping inaccessible data disk '%s'...\n", buffer); } else { /* LCOV_EXCL_START */ log_fatal(errno, "Error accessing 'disk' '%s' specification in '%s' at line %u\n", dir, device, line); @@ -6387,7 +6387,7 @@ int state_progress(struct snapraid_state* state, struct snapraid_io* io, block_o msg_bar("\n"); msg_flush(); } - log_fatal(EUSER, "Stopping for interruption at block %" PRIu64 "\n", blockpos); + log_error(EUSER, "Stopping for interruption at block %" PRIu64 "\n", blockpos); log_tag("sigint:%" PRIu64 ": SIGINT received\n", blockpos); log_flush(); return 1; diff --git a/cmdline/state.h b/cmdline/state.h index 44d89573..209df4ad 100644 --- a/cmdline/state.h +++ b/cmdline/state.h @@ -263,7 +263,7 @@ void state_scan(struct snapraid_state* state); /** * Set the nanosecond timestamp of all files that have a zero value. */ -void state_touch(struct snapraid_state* state); +int state_touch(struct snapraid_state* state); /** * Map devices if not already done. diff --git a/cmdline/support.c b/cmdline/support.c index bc7fe19d..907da2a2 100644 --- a/cmdline/support.c +++ b/cmdline/support.c @@ -181,6 +181,8 @@ uint64_t random_u64(void) int is_hw(int err) { /* LCOV_EXCL_START */ + if (err == EDATA) + return 1; if (err == EIO) return 1; #ifdef EFSCORRUPTED @@ -211,6 +213,19 @@ int is_hw(int err) return 0; } +/** + * Return if the error is a hardware error represented by a system errno. + * + * Unlike is_hw(), this excludes synthetic errors such as EDATA, which + * may be reported while still allowing a zero process exit code. + * + * All other hardware errors must prevent a successful process exit. + */ +int is_hw_errno(int err) +{ + return err > 0 && is_hw(err); +} + void log_fatal_errno(int err, const char* name) { /* LCOV_EXCL_START */ @@ -297,6 +312,18 @@ int msg_fatal_stderr = 1; FILE* stdlog = 0; int msg_line_prev = 0; /**< Previous line width on stdout */ int msg_line_curr = 0; /**< Current line width on stdout */ +static unsigned log_hardware_error_count = 0; + +unsigned log_hardware_errors(void) +{ + unsigned count; + + lock_msg(); + count = log_hardware_error_count; + unlock_msg(); + + return count; +} /* * Note that in the following functions we always flush both @@ -379,12 +406,14 @@ void vlog_fatal(int err, const char* format, va_list ap, const char* post) lock_msg(); + if (is_hw(err)) + ++log_hardware_error_count; + if (stdlog) { if (is_hw(err)) log_failed = fprintf(stdlog, "msg:fatal_hardware: ") < 0; else log_failed = fprintf(stdlog, "msg:fatal: ") < 0; - va_list ap_copy; va_copy(ap_copy, ap); if (vfprintf(stdlog, format, ap_copy) < 0) @@ -427,6 +456,9 @@ void vlog_error(int err, const char* format, va_list ap, const char* post) { lock_msg(); + if (is_hw(err)) + ++log_hardware_error_count; + if (stdlog) { if (is_hw(err)) fprintf(stdlog, "msg:error_hardware: "); @@ -470,6 +502,9 @@ void vlog_info(int err, const char* format, va_list ap, const char* post) { lock_msg(); + if (is_hw(err)) + ++log_hardware_error_count; + if (stdlog) { if (is_hw(err)) fprintf(stdlog, "msg:error_hardware: "); /* we never expect an hardware error */ @@ -536,6 +571,9 @@ void log_expected(int err, const char* format, ...) lock_msg(); + if (is_hw(err)) + ++log_hardware_error_count; + if (stdlog) { if (is_hw(err)) fprintf(stdlog, "msg:error_hardware: "); /* we never expect an hardware error */ diff --git a/cmdline/support.h b/cmdline/support.h index 98af27cd..995901f8 100644 --- a/cmdline/support.h +++ b/cmdline/support.h @@ -73,6 +73,11 @@ uint64_t random_u64(void); */ int is_hw(int err); +/** + * Return the number of hardware errors logged so far. + */ +unsigned log_hardware_errors(void); + /** * Log errors with standard messages */ diff --git a/cmdline/sync.c b/cmdline/sync.c index cfb58f82..3c7450bd 100644 --- a/cmdline/sync.c +++ b/cmdline/sync.c @@ -1767,7 +1767,7 @@ end: *sync_durable = 1; if (state->opt.kill_after_sync) { - log_fatal(EUSER, "WARNING! Killing due --test-kill-after-sync option.\n"); + log_error(EUSER, "WARNING! Killing due --test-kill-after-sync option.\n"); exit(EXIT_SUCCESS); } @@ -2029,7 +2029,7 @@ int state_sync(struct snapraid_state* state, block_off_t blockstart, block_off_t /* if the file is too small */ if (out_size < used_parity_size) { - log_fatal(ESOFT, "WARNING! The %s parity has only %" PRIu64 " blocks instead of %" PRIu64 ".\n", lev_name(l), parityblocks, used_paritymax); + log_error(ESOFT, "WARNING! The %s parity has only %" PRIu64 " blocks instead of %" PRIu64 ".\n", lev_name(l), parityblocks, used_paritymax); } /* keep the smallest physical parity extent */ @@ -2152,7 +2152,7 @@ int state_sync(struct snapraid_state* state, block_off_t blockstart, block_off_t } if (state->opt.kill_after_resize) { - log_fatal(EUSER, "WARNING! Killing due --test-kill-after-resize option.\n"); + log_error(EUSER, "WARNING! Killing due --test-kill-after-resize option.\n"); exit(EXIT_SUCCESS); } @@ -2183,7 +2183,7 @@ int state_sync(struct snapraid_state* state, block_off_t blockstart, block_off_t if (state->need_write) state_write(state); } else { - log_fatal(EUSER, "WARNING! Skipped state write for --test-skip-content-write option.\n"); + log_error(EUSER, "WARNING! Skipped state write for --test-skip-content-write option.\n"); } /* make the scanned state available for recovery before changing parity */ @@ -2193,7 +2193,7 @@ int state_sync(struct snapraid_state* state, block_off_t blockstart, block_off_t /* continue, as we are already exiting */ /* LCOV_EXCL_STOP */ } else if (state->opt.kill_before_sync) { - log_fatal(EUSER, "WARNING! Killing due --test-kill-before-sync option.\n"); + log_error(EUSER, "WARNING! Killing due --test-kill-before-sync option.\n"); exit(EXIT_SUCCESS); } else if (blockstart < blockmax) { ret = state_sync_process(state, parity_handle, blockstart, blockmax, &sync_durable); @@ -2272,13 +2272,18 @@ int state_sync(struct snapraid_state* state, block_off_t blockstart, block_off_t if (sync_durable) { state_write(state); } else { - log_fatal(EUSER, "WARNING! Skipping saving the content file because the sync was not durable.\n"); + log_error(EUSER, "WARNING! Skipping saving the content file because the sync was not durable.\n"); } } if (process_error != 0) return -1; + if (log_hardware_errors() != 0) { + msg_status("Everything done, but hardware errors were encountered in the process\n"); + return -1; + } + msg_status("Everything OK\n"); return 0; diff --git a/cmdline/touch.c b/cmdline/touch.c index 1e22a852..2b966e34 100644 --- a/cmdline/touch.c +++ b/cmdline/touch.c @@ -8,10 +8,11 @@ #include "state.h" #include "handle.h" -void state_touch(struct snapraid_state* state) +int state_touch(struct snapraid_state* state) { tommy_node* i; unsigned counter = 0; + int error = 0; msg_progress("Setting sub-second timestamps...\n"); @@ -80,6 +81,7 @@ void state_touch(struct snapraid_state* state) f = open(path, flags); if (windows_set_file_attributes(path, attributes) != 0) { log_error(errno, "Error restoring read-only attribute for '%s'. %s.\n", path, strerror(errno)); + ++error; } } } @@ -90,6 +92,7 @@ void state_touch(struct snapraid_state* state) if (f == -1) { /* LCOV_EXCL_START */ log_error(errno, "Error opening file '%s'. %s.\n", path, strerror(errno)); + ++error; continue; /* LCOV_EXCL_STOP */ } @@ -103,6 +106,7 @@ void state_touch(struct snapraid_state* state) /* LCOV_EXCL_START */ close(f); log_error(errno, "Error accessing file '%s'. %s.\n", path, strerror(errno)); + ++error; continue; /* LCOV_EXCL_STOP */ } @@ -113,6 +117,7 @@ void state_touch(struct snapraid_state* state) /* LCOV_EXCL_START */ close(f); log_error(errno, "Error timing file '%s'. %s.\n", path, strerror(errno)); + ++error; continue; /* LCOV_EXCL_STOP */ } @@ -127,6 +132,7 @@ void state_touch(struct snapraid_state* state) /* LCOV_EXCL_START */ close(f); log_error(errno, "Error accessing file '%s'. %s.\n", path, strerror(errno)); + ++error; continue; /* LCOV_EXCL_STOP */ } @@ -136,6 +142,7 @@ void state_touch(struct snapraid_state* state) if (ret != 0) { /* LCOV_EXCL_START */ log_error(errno, "Error closing file '%s'. %s.\n", path, strerror(errno)); + ++error; continue; /* LCOV_EXCL_STOP */ } @@ -161,5 +168,9 @@ void state_touch(struct snapraid_state* state) msg_status("\n"); msg_status("%8u touched files\n", counter); + + if (error != 0) + return -1; + return 0; } diff --git a/cmdline/unixapp.c b/cmdline/unixapp.c index b6e87d61..371061d2 100644 --- a/cmdline/unixapp.c +++ b/cmdline/unixapp.c @@ -4124,7 +4124,7 @@ int devquery(tommy_list* high, tommy_list* low) /* sysfs interface is required */ if (stat("/sys/dev/block", &st) != 0) { /* LCOV_EXCL_START */ - log_fatal(EEXTERNAL, "Missing interface /sys/dev/block.\n"); + log_error(EEXTERNAL, "Missing interface /sys/dev/block.\n"); return -1; /* LCOV_EXCL_STOP */ }