From 22e9e00f632513439dd890da43ae6ea873d2a742 Mon Sep 17 00:00:00 2001 From: Andrew Wood Date: Mon, 27 Oct 2025 20:41:32 +0000 Subject: [PATCH] Move the array of watchfd item information into the main state, instead of being local to the watchfd loop, so that in future we can add stoppage time to individual fd info and it will be accessible to the signal handler (#169). --- src/include/pv-internal.h | 80 ++++++++++++++++++++--------------- src/pv/loop.c | 58 +++++-------------------- src/pv/state.c | 89 +++++++++++++++++++++++++-------------- src/pv/watchpid.c | 23 ++++++++-- 4 files changed, 134 insertions(+), 116 deletions(-) diff --git a/src/include/pv-internal.h b/src/include/pv-internal.h index e2772fd..9c18ed2 100644 --- a/src/include/pv-internal.h +++ b/src/include/pv-internal.h @@ -131,6 +131,12 @@ typedef uint16_t pvdisplay_bytecount_t; typedef uint16_t pvdisplay_width_t; #define PVDISPLAY_WIDTH_MAX (65535) /* UINT16_MAX */ +/* + * Structure defining the current state of a single watched file descriptor. + */ +struct pvwatchfd_s; +typedef /*@null@*/ struct pvwatchfd_s *pvwatchfd_t; + /* String pointer, that is the only pointer to this resource, that can be null. */ typedef /*@only@*/ /*@null@*/ char * nullable_string_t; @@ -163,9 +169,16 @@ struct pvstate_s { /********************************* * Items to watch with --watchfd * *********************************/ - struct pvwatchspec_s { - /*@only@*/ /*@null@*/ pid_t *pid; /* array of processes to watch fds of */ - /*@only@*/ /*@null@*/ int *fd; /* array of fds to watch in each one (0=all) */ + struct { + /*@only@*/ /*@null@*/ + struct pvwatcheditem_s { /* array of PID or PID:FD items */ + pid_t pid; /* watched PID */ + int fd; /* watched fd, or -1 for all */ + /*@null@*/ + pvwatchfd_t info_array; /* watch information for each fd */ + int array_length; /* length of watch info array */ + bool finished; /* "PID:FD": fd closed; or PID gone */ + } *watching; unsigned int count; /* number of items in these arrays */ bool multiple_pids; /* true if more than one distinct PID */ } watchfd; @@ -468,6 +481,36 @@ typedef struct pvtransfercalc_s *pvtransfercalc_t; typedef struct pvcursorstate_s *pvcursorstate_t; typedef struct pvtransferstate_s *pvtransferstate_t; +/* + * Structure defining the current state of a single watched file descriptor. + * The full definition needs to go here as it refers to sub-structures of + * the main state, defined above. + */ +struct pvwatchfd_s { + struct pvtransientflags_s flags; /* transient flags */ + struct pvtransferstate_s transfer; /* transfer state */ + struct pvtransfercalc_s calc; /* calculated transfer state */ + struct pvdisplay_s display; /* display data */ +#ifdef __APPLE__ +#else + char file_fdinfo[PV_SIZEOF_FILE_FDINFO]; /* path to /proc fdinfo file */ + char file_fd[PV_SIZEOF_FILE_FD]; /* path to /proc fd symlink */ +#endif + char file_fdpath[PV_SIZEOF_FILE_FDPATH]; /* path to file that was opened */ + /*@keep@ */ char display_name[PV_SIZEOF_DISPLAY_NAME]; /* name to show on progress bar */ + struct stat sb_fd; /* stat of fd symlink */ + struct stat sb_fd_link; /* lstat of fd symlink */ + off_t size; /* size of whole file, 0 if unknown */ + off_t position; /* position last seen at */ + struct timespec start_time; /* time we started watching the fd */ + struct timespec end_time; /* time the fd was marked as closed */ + pid_t watch_pid; /* PID the fd belongs to */ + int watch_fd; /* fd to watch */ + bool closed; /* true once the fd is closed */ + bool displayable; /* false if not displayable */ + bool unused; /* true if free for re-use */ +}; + /* * Read-only counterparts to the above structure pointers, to be used in * function declarations where the function definitely shouldn't be altering @@ -514,36 +557,6 @@ struct pvdisplay_component_s { bool dynamic; /* whether it can scale with screen size */ }; - -/* - * Structure defining the current state of a single watched file descriptor. - */ -struct pvwatchfd_s { - struct pvtransientflags_s flags; /* transient flags */ - struct pvtransferstate_s transfer; /* transfer state */ - struct pvtransfercalc_s calc; /* calculated transfer state */ - struct pvdisplay_s display; /* display data */ -#ifdef __APPLE__ -#else - char file_fdinfo[PV_SIZEOF_FILE_FDINFO]; /* path to /proc fdinfo file */ - char file_fd[PV_SIZEOF_FILE_FD]; /* path to /proc fd symlink */ -#endif - char file_fdpath[PV_SIZEOF_FILE_FDPATH]; /* path to file that was opened */ - /*@keep@ */ char display_name[PV_SIZEOF_DISPLAY_NAME]; /* name to show on progress bar */ - struct stat sb_fd; /* stat of fd symlink */ - struct stat sb_fd_link; /* lstat of fd symlink */ - off_t size; /* size of whole file, 0 if unknown */ - off_t position; /* position last seen at */ - struct timespec start_time; /* time we started watching the fd */ - struct timespec end_time; /* time the fd was marked as closed */ - pid_t watch_pid; /* PID the fd belongs to */ - int watch_fd; /* fd to watch */ - bool closed; /* true once the fd is closed */ - bool displayable; /* false if not displayable */ - bool unused; /* true if free for re-use */ -}; -typedef struct pvwatchfd_s *pvwatchfd_t; - void pv_error(char *, ...); int pv_main_loop(pvstate_t); @@ -620,6 +633,7 @@ void pv_freecontents_display(pvdisplay_t); void pv_freecontents_transfer(pvtransferstate_t); void pv_freecontents_calc(pvtransfercalc_t); void pv_freecontents_watchfd(pvwatchfd_t); +void pv_freecontents_watchfd_items(struct pvwatcheditem_s *, unsigned int); void pv_write_retry(int, const char *, size_t); void pv_tty_write(readonly_pvtransientflags_t, const char *, size_t); diff --git a/src/pv/loop.c b/src/pv/loop.c index 1ea374b..cf25007 100644 --- a/src/pv/loop.c +++ b/src/pv/loop.c @@ -694,9 +694,7 @@ static void pv_watchfd_update_format_string(pvstate_t state) /* If there's nothing to watch, do nothing. */ if (state->watchfd.count < 1) return; - if (NULL == state->watchfd.pid) - return; - if (NULL == state->watchfd.fd) + if (NULL == state->watchfd.watching) return; /* @@ -712,7 +710,7 @@ static void pv_watchfd_update_format_string(pvstate_t state) memset(new_format_string, 0, sizeof(new_format_string)); - if (state->watchfd.count > 1 || -1 == state->watchfd.fd[0]) { + if (state->watchfd.count > 1 || -1 == state->watchfd.watching[0].fd) { /* Watching more than one single FD; need %N. */ if (!format_contains_name(original_format_string)) { (void) pv_snprintf(new_format_string, sizeof(new_format_string), "%%N %s", @@ -753,13 +751,7 @@ static void pv_watchfd_update_format_string(pvstate_t state) */ int pv_watchfd_loop(pvstate_t state) { - struct { /* details for each PID:FD specified by operator */ - pid_t pid; /* watched PID */ - int fd; /* watched fd, or -1 for all */ - pvwatchfd_t info_array; /* watch information for each fd */ - int array_length; /* length of watch info array */ - bool finished; /* "PID:FD": fd closed; or PID gone */ - } *watching; + struct pvwatcheditem_s *watching; unsigned int watch_idx; bool all_watching_finished; struct timespec next_update, next_remotecheck, cur_time; @@ -776,11 +768,12 @@ int pv_watchfd_loop(pvstate_t state) /* If there's nothing to watch, do nothing at all. */ if (state->watchfd.count < 1) return 0; - if (NULL == state->watchfd.pid) - return PV_ERROREXIT_MEMORY; - if (NULL == state->watchfd.fd) + if (NULL == state->watchfd.watching) return PV_ERROREXIT_MEMORY; + /* Local alias for less cumbersome access. */ + watching = state->watchfd.watching; + /* * Make sure there's no name set. */ @@ -792,16 +785,6 @@ int pv_watchfd_loop(pvstate_t state) /* Adjust the format string so PID:FD or FD prefixes are shown. */ pv_watchfd_update_format_string(state); - /* - * Allocate the watching array. - */ - watching = malloc(state->watchfd.count * sizeof(*watching)); - if (NULL == watching) { - pv_error("%s: %s", _("buffer allocation failed"), strerror(errno)); - return PV_ERROREXIT_MEMORY; - } - memset(watching, 0, state->watchfd.count * sizeof(*watching)); - /* * Populate the watching array. In the process of doing so, raise * errors if any of the initially specified PIDs or PID:FD pairs @@ -810,8 +793,6 @@ int pv_watchfd_loop(pvstate_t state) for (watch_idx = 0; watch_idx < state->watchfd.count; watch_idx++) { int rc; - watching[watch_idx].pid = state->watchfd.pid[watch_idx]; - watching[watch_idx].fd = state->watchfd.fd[watch_idx]; watching[watch_idx].info_array = NULL; watching[watch_idx].array_length = 0; watching[watch_idx].finished = false; @@ -849,7 +830,7 @@ int pv_watchfd_loop(pvstate_t state) /* Scan failed - mark as finished. */ state->status.exit_status |= PV_ERROREXIT_ACCESS; watching[watch_idx].finished = true; - } else if (0 == watching[watch_idx].array_length) { + } else if ((0 == watching[watch_idx].array_length) || (NULL == watching[watch_idx].info_array)) { /* FD not found - error, mark as finished. */ pv_error("%s %u: %s %d: %s", _("pid"), watching[watch_idx].pid, _("fd"), watching[watch_idx].fd, @@ -1189,29 +1170,10 @@ int pv_watchfd_loop(pvstate_t state) state->status.exit_status |= PV_ERROREXIT_SIGNAL; end_pv_watchfd_loop: - /* Free all allocated memory. */ - /*@-compdestroy@ */ - for (watch_idx = 0; NULL != watching && watch_idx < state->watchfd.count; watch_idx++) { - int info_idx; - if (NULL == watching[watch_idx].info_array) - continue; - for (info_idx = 0; info_idx < watching[watch_idx].array_length; info_idx++) { - pv_freecontents_watchfd(&(watching[watch_idx].info_array[info_idx])); - } - free(watching[watch_idx].info_array); - watching[watch_idx].info_array = NULL; - watching[watch_idx].array_length = 0; - } - free(watching); - watching = NULL; + /* Free all allocated sub-structures. */ + pv_freecontents_watchfd_items(watching, state->watchfd.count); return state->status.exit_status; - /*@+compdestroy @ */ - /* - * splint warns that watching[].info_array->transfer.transfer_buffer - * and other deep structures may not be deallocated and so leak - * memory, but that's what pv_freecontents_watchfd() takes care of. - */ } diff --git a/src/pv/state.c b/src/pv/state.c index a5c877d..ba30016 100644 --- a/src/pv/state.c +++ b/src/pv/state.c @@ -257,6 +257,39 @@ void pv_freecontents_calc(pvtransfercalc_t calc) } +/* + * Free the contents of a watchfd watched-items array. + */ +void pv_freecontents_watchfd_items(struct pvwatcheditem_s *watching, unsigned int count) +{ + unsigned int watch_idx; + + if (NULL == watching) + return; + + /*@-compdestroy@ */ + for (watch_idx = 0; watch_idx < count; watch_idx++) { + int info_idx; + if (NULL == watching[watch_idx].info_array) + continue; + for (info_idx = 0; info_idx < watching[watch_idx].array_length; info_idx++) { + pv_freecontents_watchfd(&(watching[watch_idx].info_array[info_idx])); + } + free(watching[watch_idx].info_array); + watching[watch_idx].info_array = NULL; + watching[watch_idx].array_length = 0; + } + + return; + /*@+compdestroy @ */ + /* + * splint warns that watching[].info_array->transfer.transfer_buffer + * and other deep structures may not be deallocated and so leak + * memory, but that's what pv_freecontents_watchfd() takes care of. + */ +} + + /* * Truncate the output file descriptor to its current position, if it's a * valid fd, we're in sparse output mode, and no lseek() failed. @@ -362,13 +395,10 @@ void pv_state_free(pvstate_t state) state->files.filename = NULL; } - if (NULL != state->watchfd.pid) { - free(state->watchfd.pid); - state->watchfd.pid = NULL; - } - if (NULL != state->watchfd.fd) { - free(state->watchfd.fd); - state->watchfd.fd = NULL; + if (NULL != state->watchfd.watching) { + pv_freecontents_watchfd_items(state->watchfd.watching, state->watchfd.count); + free(state->watchfd.watching); + state->watchfd.watching = NULL; } free(state); @@ -788,43 +818,38 @@ void pv_state_inputfiles(pvstate_t state, unsigned int input_file_count, const c void pv_state_watchfds(pvstate_t state, unsigned int watchfd_count, const pid_t * pids, const int *fds) { unsigned int item_idx; - /*@only@ */ pid_t *new_pid_array = NULL; - /*@only@ */ int *new_fd_array = NULL; + /*@only@ */ struct pvwatcheditem_s *new_array = NULL; /* Free the old arrays, if there were any. */ - if (NULL != state->watchfd.pid) { - free(state->watchfd.pid); - state->watchfd.pid = NULL; - } - if (NULL != state->watchfd.fd) { - free(state->watchfd.fd); - state->watchfd.fd = NULL; + if (NULL != state->watchfd.watching) { + /*@-compdestroy@ */ + pv_freecontents_watchfd_items(state->watchfd.watching, state->watchfd.count); + free(state->watchfd.watching); + state->watchfd.watching = NULL; + /*@+compdestroy@ */ + /* + * splint warns about deep structures not being deallocated, + * but that's what pv_freecontents_watchfd_items() does. + */ } state->watchfd.count = 0; state->watchfd.multiple_pids = false; - /* Allocate empty new arrays of the right size. */ - new_pid_array = calloc((size_t) (watchfd_count + 1), sizeof(pid_t)); - if (NULL == new_pid_array) { + /* Allocate an empty new array of the right size. */ + new_array = malloc((1 + state->watchfd.count) * sizeof(*new_array)); + if (NULL == new_array) { /*@-mustfreefresh@ *//* see similar _() issue above */ - pv_error("%s: %s", _("process list allocation failed"), strerror(errno)); + pv_error("%s: %s", _("buffer allocation failed"), strerror(errno)); /*@+mustfreefresh@ */ return; } - state->watchfd.pid = new_pid_array; - new_fd_array = calloc((size_t) (watchfd_count + 1), sizeof(int)); - if (NULL == new_fd_array) { - /*@-mustfreefresh@ *//* see similar _() issue above */ - pv_error("%s: %s", _("file descriptor list allocation failed"), strerror(errno)); - /*@+mustfreefresh@ */ - return; - } - state->watchfd.fd = new_fd_array; + memset(new_array, 0, (1 + state->watchfd.count) * sizeof(*new_array)); + state->watchfd.watching = new_array; - /* Populate the new arrays with the values supplied. */ + /* Populate the new array with the values supplied. */ for (item_idx = 0; item_idx < watchfd_count; item_idx++) { - state->watchfd.pid[item_idx] = pids[item_idx]; - state->watchfd.fd[item_idx] = fds[item_idx]; + state->watchfd.watching[item_idx].pid = pids[item_idx]; + state->watchfd.watching[item_idx].fd = fds[item_idx]; if ((item_idx > 0) && (pids[item_idx] != pids[item_idx - 1])) state->watchfd.multiple_pids = true; } diff --git a/src/pv/watchpid.c b/src/pv/watchpid.c index e20e3e5..7ad2c72 100644 --- a/src/pv/watchpid.c +++ b/src/pv/watchpid.c @@ -42,6 +42,8 @@ */ static bool filesize(pvwatchfd_t info) { + if (NULL == info) + return false; if (S_ISBLK(info->sb_fd.st_mode)) { int fd; @@ -222,6 +224,9 @@ bool pv_watchfd_changed(pvwatchfd_t info) { struct stat sb_fd, sb_fd_link; + if (NULL == info) + return false; + memset(&sb_fd, 0, sizeof(sb_fd)); memset(&sb_fd_link, 0, sizeof(sb_fd_link)); @@ -265,6 +270,9 @@ off_t pv_watchfd_position(pvwatchfd_t info) long long pos_long; FILE *fptr; + if (NULL == info) + return -1; + if (pv_watchfd_changed(info)) return -1; @@ -489,7 +497,7 @@ int pv_watchpid_scanfds(pvstate_t state, pid_t watch_pid, int watch_fd, int *arr * Skip if this fd is already known to us. */ found_idx = -1; - for (check_idx = 0; check_idx < array_length; check_idx++) { + for (check_idx = 0; check_idx < array_length && NULL != info_array; check_idx++) { if (info_array[check_idx].unused) continue; if (info_array[check_idx].watch_fd != fd) @@ -514,7 +522,7 @@ int pv_watchpid_scanfds(pvstate_t state, pid_t watch_pid, int watch_fd, int *arr * See if there's an empty slot we can re-use. */ use_idx = -1; - for (check_idx = 0; check_idx < array_length; check_idx++) { + for (check_idx = 0; check_idx < array_length && NULL != info_array; check_idx++) { if (info_array[check_idx].unused) { use_idx = check_idx; break; @@ -532,6 +540,10 @@ int pv_watchpid_scanfds(pvstate_t state, pid_t watch_pid, int watch_fd, int *arr use_idx = array_length - 1; } + /* At this point, the array should exist. */ + if (NULL == info_array) + return 2; + debug("%s: %d => index %d", "found new fd", fd, use_idx); changes_made = true; @@ -638,7 +650,12 @@ void pv_watchpid_setname(pvstate_t state, pvwatchfd_t info) { size_t path_length, cwd_length; int max_display_length; - char *file_fdpath = info->file_fdpath; + char *file_fdpath; + + if (NULL == info) + return; + + file_fdpath = info->file_fdpath; memset(info->display_name, 0, PV_SIZEOF_DISPLAY_NAME);