diff --git a/src/main/debug.c b/src/main/debug.c index 42ad4d9..68b8274 100644 --- a/src/main/debug.c +++ b/src/main/debug.c @@ -44,16 +44,16 @@ void debugging_output(const char *function, const char *file, int line, const ch /* * flawfinder note: tbuf is only written to by strftime() which - * takes its size, and we enforce string termination. + * takes its size, and string termination is enforced. */ if (false == tried_open) { if (NULL != debug_filename) { debugfptr = fopen(debug_filename, "a"); /* flawfinder: ignore */ /* - * flawfinder note: caller directly controls - * filename, the safest we can manage is to use - * append mode. + * flawfinder note: the caller directly controls the + * filename, so using append mode is the safest + * option. */ } tried_open = true; @@ -64,10 +64,9 @@ void debugging_output(const char *function, const char *file, int line, const ch } /* - * Note that here we use gmtime() rather than localtime(), otherwise - * we can get stuck in signal handlers - testing with "strace" - * showed many cases where "pv /dev/null" being - * paused and backgrounded would cause pv to be stuck in + * Note gmtime() is used rather than localtime(). Testing with + * "strace" showed many cases where "pv /dev/null" + * being paused and backgrounded would cause pv to be stuck in * futex_wait() inside a pv_sig_alrm() inside a pv_sig_cont(). The * backtrace mentioned many time zone conversion steps, and all of * that goes away with gmtime(). @@ -79,7 +78,7 @@ void debugging_output(const char *function, const char *file, int line, const ch if (0 == strftime(tbuf, sizeof(tbuf), "%Y-%m-%d %H:%M:%S", tm)) { tbuf[0] = '\0'; } - tbuf[sizeof(tbuf) - 1] = '\0'; /* enforce termination */ + tbuf[sizeof(tbuf) - 1] = '\0'; /* enforce termination. */ (void) fprintf(debugfptr, "[%s] (%d) %s (%s:%d): ", tbuf, getpid(), function, file, line); @@ -89,7 +88,7 @@ void debugging_output(const char *function, const char *file, int line, const ch /* * flawfinder note: vfprintf format is explicitly controlled by the - * caller of this function - no mitigation possible or desirable. + * caller of this function - no mitigation is possible or desirable. */ (void) fprintf(debugfptr, "\n"); diff --git a/src/main/help.c b/src/main/help.c index b7acc16..ea04225 100644 --- a/src/main/help.c +++ b/src/main/help.c @@ -37,9 +37,8 @@ static size_t display_width(const char *string) bytes = strlen(string); /* flawfinder: ignore */ /* - * flawfinder rationale: we have already checked for NULL, and it is - * explicitly required of the caller to provide a null-terminated - * string. + * flawfinder rationale: it is explicitly required of the caller to + * provide a null-terminated string. */ return pv_strwidth(string, bytes); @@ -175,7 +174,7 @@ static void display_word_wrap(const char *string, size_t display_width, size_t f /* Wrap lines that are too long. */ - /*@-unrecog@ *//* splint seems unable to see the prototype for wcswidth(). */ + /*@-unrecog@ *//* splint doesn't see the prototype for wcswidth(). */ while (chars_remaining > 0 && wcswidth(&(wide_string[start_idx]), chars_remaining) > (int) wrap_at_width) { /*@+unrecog@ */ size_t next_idx; @@ -203,9 +202,9 @@ static void display_word_wrap(const char *string, size_t display_width, size_t f while (start_idx < end_idx && start_idx < wide_char_count) { char multi_byte_string[MB_CUR_MAX + 1]; /* flawfinder: ignore */ /* - * flawfinder rationale: array is explicitly + * flawfinder rationale: the array is explicitly * cleared, large enough according to the wctomb() - * manual, and we explicitly terminate the string. + * manual, and the string is explicitly terminated. */ memset(multi_byte_string, 0, MB_CUR_MAX + 1); if (wctomb(multi_byte_string, wide_string[start_idx]) >= 0) { @@ -487,7 +486,7 @@ void display_help(void) /* * splint note: the gettext calls made by _() cause memory leak * warnings, but in this case it's unavoidable, and mitigated by the - * fact we only translate each string once. + * fact that each string is only translated once. */ program_description = _("Concatenate FILE(s), or standard input, to standard output, with monitoring."); if (NULL != program_description) { @@ -501,8 +500,8 @@ void display_help(void) * Translate the help text, and calculate the displayed width of * each part of each option definition. The total display width of * the short option, long option, and option argument together form - * the "option" width - we look for the widest one to calculate the - * left margin for all of the descriptions to start at. + * the "option" width. The widest one is used to calculate the left + * margin for all of the descriptions to start at. */ for (option_index = 0; NULL != option_definitions[option_index].opt_short; option_index++) { struct option_definition_s *definition; @@ -546,7 +545,8 @@ void display_help(void) * * " , " * - * If we don't have getopt_long() then ", " is omitted. + * If getopt_long() is unavailable then ", " is + * omitted. */ option_width += 2 + definition->width.opt_short; /* " short" */ #ifdef HAVE_GETOPT_LONG @@ -611,7 +611,7 @@ void display_help(void) /* * If the option (with 2 trailing spaces) is too wide, start - * a new line for the description. In both cases, pad with + * a new line for the description. Either way, pad with * spaces up to the description left margin. */ if ((option_width + 2) > description_left_margin) { diff --git a/src/main/main.c b/src/main/main.c index 7dcb377..2b983bf 100644 --- a/src/main/main.c +++ b/src/main/main.c @@ -44,20 +44,18 @@ static int pv__write_pidfile(opts_t opts) /* * The buffer needs to be long enough to hold the pidfile with the - * mkstemp template ".XXXXXX" after it. The "%s" of our - * pidfile_template adds 2 extra bytes to the length, of which we - * need 1 byte for the terminating \0, so we subtract 1 byte more to - * get the exact amount of space we need. + * mkstemp template ".XXXXXX" after it. The "%s" of the + * pidfile_template adds 2 extra bytes to the length, of which 1 + * byte is used for the terminating \0, so subtract 1 byte more to + * get the exact amount of space needed. */ pidfile_tmp_bufsize = strlen(pidfile_template) + strlen(opts->pidfile) - 1; /* flawfinder: ignore */ /* - * flawfinder rationale: flawfinder never likes strlen() in case - * it's called on a string that isn't \0 terminated. We have to use - * strlen() to find the length of opts->pidfile, so have to trust - * that the arguments in argv[] were \0 terminated. We can be sure - * that pidfile_template is \0 terminated because we've set it to a - * constant value. So we tell flawfinder to skip this check here. + * flawfinder rationale: the warning is about strlen() being called + * on a string that may not be \0 terminated. pidfile_template is a + * constant so is always \0-terminated. opts->pidfile comes from + * argv[] whose entries are \0-terminated. */ pidfile_tmp_name = malloc(pidfile_tmp_bufsize); @@ -68,11 +66,11 @@ static int pv__write_pidfile(opts_t opts) memset(pidfile_tmp_name, 0, pidfile_tmp_bufsize); (void) pv_snprintf(pidfile_tmp_name, pidfile_tmp_bufsize, pidfile_template, opts->pidfile); - /*@-type@ *//* splint doesn't like mode_t */ + /*@-type@ *//* splint doesn't like mode_t. */ prev_umask = umask(0000); /* flawfinder: ignore */ (void) umask(prev_umask | 0133); /* flawfinder: ignore */ - /*@-unrecog@ *//* splint doesn't know mkstemp() */ + /*@-unrecog@ *//* splint doesn't know mkstemp(). */ pidfile_tmp_fd = mkstemp(pidfile_tmp_name); /* flawfinder: ignore */ /*@+unrecog@ */ if (pidfile_tmp_fd < 0) { @@ -85,14 +83,11 @@ static int pv__write_pidfile(opts_t opts) (void) umask(prev_umask); /* flawfinder: ignore */ /* - * flawfinder rationale (umask, mkstemp) - flawfinder - * recommends setting the most restrictive umask possible - * when calling mkstemp(), so this is what we have done. - * - * We get the original umask and OR it with 0133 to make - * sure new files will be at least chmod 644. Then we put - * the umask back to what it was, after creating the - * temporary file. + * flawfinder rationale (umask, mkstemp) - flawfinder recommends + * setting the most restrictive umask possible when calling + * mkstemp(). To do this, the original umask is ORed with 0133 to + * ensure that new files will be chmod 644 or more restricted. + * After creating the temporary file, the old umask is restored. */ /*@+type@ */ @@ -143,11 +138,10 @@ static int pv__set_output(pvstate_t state, opts_t opts, /*@null@ */ const char * debug("%s: %s", "setting output", output_file); output_fd = open(output_file, O_WRONLY | O_CREAT | O_TRUNC, 0600); /* flawfinder: ignore */ /* - * flawfinder rationale: the output filename has been - * explicitly provided, and in many cases the operator will - * want to write to device files and other special - * destinations, so there is no sense-checking we can do to - * make this safer. + * flawfinder rationale: the output filename has been explicitly + * provided, and in many cases the operator will want to write to + * device files and other special destinations, so there is no + * checking that could be done to make this safer. */ if (output_fd < 0) { fprintf(stderr, "%s: %s: %s\n", opts->program_name, output_file, strerror(errno)); @@ -202,7 +196,7 @@ static int pv__store_and_forward(pvstate_t state, opts_t opts, pvformatoptions_s */ (void) pv_snprintf(tmp_filename, sizeof(tmp_filename), "%s/pv.XXXXXX", tmpdir); - /*@-unrecog@ *//* splint doesn't know mkstemp() */ + /*@-unrecog@ *//* splint doesn't know mkstemp(). */ tmp_fd = mkstemp(tmp_filename); /* flawfinder: ignore */ /*@+unrecog@ */ if (tmp_fd < 0) { @@ -253,7 +247,7 @@ static int pv__store_and_forward(pvstate_t state, opts_t opts, pvformatoptions_s /* Set the displayed name to whatever was requested. */ pv_state_name_set(state, opts->name); - /* Reset the format, since we might have been asked to show ETA. */ + /* Reset the format, in case ETA was originally requested. */ format_options.eta = opts->eta; format_options.fineta = opts->fineta; pv_state_set_format_options(state, format_options); @@ -338,7 +332,7 @@ static int pv__run_monitor(const char *program_name, pvstate_t state, pvside_t s debug("%s: %llu", "no size given - calculated", size); } - /* If the size is unknown, we cannot have an ETA. */ + /* If the size is unknown, ETA cannot be displayed. */ if (size < 1) { format_options.eta = false; format_options.fineta = false; @@ -400,7 +394,7 @@ static int pv__monitor(pvstate_t state, opts_t opts, pvformatoptions_s format_op pipefd_cmd_out[0] = -1; pipefd_cmd_out[1] = -1; - /* Pipe for the input side of the command, if we're monitoring it. */ + /* Pipe for the input side of the command, if it's to be monitored. */ if ((PV_SIDE_IN == opts->side) || (PV_SIDE_BOTH == opts->side)) { if (0 != pipe(pipefd_cmd_in)) { fprintf(stderr, "%s: %s\n", opts->program_name, strerror(errno)); @@ -409,7 +403,7 @@ static int pv__monitor(pvstate_t state, opts_t opts, pvformatoptions_s format_op debug("pipefd_cmd_in[]=(%d,%d)", pipefd_cmd_in[0], pipefd_cmd_in[1]); } - /* Pipe for the output side of the command, if we're monitoring it. */ + /* Pipe for the output side of the command, if it's to be monitored. */ if ((PV_SIDE_OUT == opts->side) || (PV_SIDE_BOTH == opts->side)) { if (0 != pipe(pipefd_cmd_out)) { fprintf(stderr, "%s: %s\n", opts->program_name, strerror(errno)); @@ -575,8 +569,8 @@ x = 1; \ /* * In "both" mode, the remaining side is "in", so if two sets of - * name and/or format options were given, we need to switch to using - * the ones given first. + * name and/or format options were given, switch to using the ones + * that had been given first. */ if (PV_SIDE_BOTH == opts->side) { if (NULL != opts->name1) { @@ -612,8 +606,7 @@ x = 1; \ /* * If monitoring the "in" side, close stdout to signal EOF, - * otherwise when we wait for the monitored command, we'll wait - * forever. + * otherwise the wait for the monitored command would wait forever. */ if (PV_SIDE_IN == opts->side || PV_SIDE_BOTH == opts->side) { if (close(STDOUT_FILENO) < 0) { @@ -672,7 +665,7 @@ int main(int argc, char **argv) (void) bindtextdomain(PACKAGE, LOCALEDIR); (void) textdomain(PACKAGE); #ifdef HAVE_LANGINFO_H - /*@-mustfreefresh@ *//* splint thinks nl_langinfo() leaks memory */ + /*@-mustfreefresh@ *//* splint thinks nl_langinfo() leaks memory. */ if (0 == strcmp(nl_langinfo(CODESET), "UTF-8")) terminal_supports_utf8 = true; /*@+mustfreefresh@ */ @@ -708,7 +701,8 @@ int main(int argc, char **argv) /* * splint note: the gettext calls made by _() cause memory * leak warnings, but in this case it's unavoidable, and - * mitigated by the fact we only translate each string once. + * mitigated by the fact that each string is only translated + * once. */ fprintf(stderr, "%s: %s: %s\n", opts->program_name, _("state allocation failed"), strerror(errno)); opts_free(opts); @@ -731,7 +725,7 @@ int main(int argc, char **argv) } /* - * If no files were given, pretend "-" was given (stdin). + * If no files were given, behave as if "-" was given (stdin). */ if (0 == opts->argc) { debug("%s", "no files given - adding fake argument `-'"); @@ -743,7 +737,7 @@ int main(int argc, char **argv) } /* - * Put our list of input files into the PV internal state. + * Put the list of input files into the PV internal state. * * Don't do this in monitor mode, since the rest of PV won't be * using the list in that case. @@ -760,8 +754,8 @@ int main(int argc, char **argv) } /* - * If stderr is not a terminal and we're neither forcing output nor - * outputting numerically, we will have nothing to display at all. + * If stderr is not a terminal and neither --force nor --numeric is + * active, there will be nothing to display at all. */ if ((0 == isatty(STDERR_FILENO)) && (false == opts->force) @@ -809,14 +803,14 @@ int main(int argc, char **argv) opts->interval = 600; /* - * Set the output file, treating no output or "-" as stdout; we have - * to do this before looking at setting the size, as the size + * Set the output file, treating no output or "-" as stdout. This + * must be done before trying to set the size, as the size * calculation looks at the output file if the input size can't be * calculated (issue #91). * - * We have to set the sparse output flag before doing this, so that - * in sparse mode the lseek() on O_APPEND can be done (issue #45); - * see the comments in pv_state_output_set() in src/pv/state.c. + * The sparse output flag before doing this, so that in sparse mode + * the lseek() on O_APPEND can be done (issue #45); see the comments + * in pv_state_output_set() in src/pv/state.c. */ pv_state_sparse_output_set(state, opts->sparse_output); retcode = pv__set_output(state, opts, opts->output); @@ -849,7 +843,7 @@ int main(int argc, char **argv) } /* - * If the size is unknown, we cannot have an ETA. + * If the size is unknown, ETA cannot be shown. */ if (opts->size < 1) { can_have_eta = false; diff --git a/src/main/options.c b/src/main/options.c index 1bd63d3..4d2ced8 100644 --- a/src/main/options.c +++ b/src/main/options.c @@ -45,7 +45,7 @@ static bool opts_watchfd_parse(opts_t, const char *, /*@null@ */ const char *, u /* * splint note about mustfreefresh: the gettext calls made by _() cause * memory leak warnings, but in these cases it's unavoidable, and mitigated - * by the fact we only translate each string once. + * by the fact that each string is only translated once. */ @@ -58,9 +58,9 @@ void opts_free( /*@only@ */ opts_t opts) return; /*@-keeptrans@ */ /* - * splint note: we're explicitly being handed the "opts" object to - * free it, so the previously "kept" internally allocated buffers - * are now ours to free. + * splint note: the "opts" object was explicitly passed to be freed, + * so the previously "kept" internally allocated buffers are no + * longer "kept". */ if (NULL != opts->name) free(opts->name); @@ -120,12 +120,11 @@ bool opts_add_file(opts_t opts, const char *filename) /*@+branchstate@ */ /* - * splint notes: we turned off "branchstate" above because depending - * on whether we have to extend the array, we change argv from - * "keep" to "only", which is also why we turned off "keeptrans"; - * there doesn't seem to be a clean way to tell splint that everyone - * else should not touch argv but we're allowed to reallocate it and - * so is opts_parse. + * splint notes: "branchstate" and "keeptrans" are turned off + * because depending on whether the array is to be extended, argv + * can change from "keep" to "only". There doesn't seem to be a + * clean way to tell splint that this function, and opts_parse(), + * are allowed to reallocate argv, but nothing else should alter it. */ opts->argv[opts->argc++] = filename; @@ -163,8 +162,8 @@ static bool opts_watchfd_add_item(opts_t opts, pid_t pid, int fd) /*@+branchstate@ */ /* - * splint notes: we turned off "branchstate" and "keeptrans" above - * because of the same reason as in opts_add_file(). + * splint notes: "branchstate" and "keeptrans" were turned off + * because of the same reason as for argv in opts_add_file(). */ opts->watchfd_pid[opts->watchfd_count] = pid; @@ -190,12 +189,6 @@ static bool opts_watchfd_processname(opts_t opts, const char *process_name) int pid_status; bool ok; - /* - * flawfinder: buffer is zeroed before each use, and fgets() is - * passed one less than its size so the string functions in the loop - * always find a null byte at the end. - */ - /* Pipe for communicating with pgrep. */ if (pipe(fds) < 0) { fprintf(stderr, "%s: %s\n", opts->program_name, strerror(errno)); @@ -270,7 +263,7 @@ static bool opts_watchfd_processname(opts_t opts, const char *process_name) /* Set errno to 0 to distinguish between EOF and error. */ errno = 0; - /*@-unrecog@ *//* splint dosn't know of getline(). */ + /*@-unrecog@ *//* splint doesn't know of getline(). */ line_length = getline(&linebuf_ptr, &linebuf_size, fptr); /*@+unrecog@ */ if ((line_length < 0) || (NULL == linebuf_ptr)) { @@ -281,7 +274,7 @@ static bool opts_watchfd_processname(opts_t opts, const char *process_name) if (line_length < 1) continue; - /* Skip all lines if we've hit any errors. */ + /* Skip all lines if any errors were found. */ if (!ok) continue; @@ -289,7 +282,7 @@ static bool opts_watchfd_processname(opts_t opts, const char *process_name) if ('\n' == linebuf_ptr[line_length - 1]) linebuf_ptr[--line_length] = '\0'; - /* Skip lines without valid PID. */ + /* Skip lines without a valid PID. */ watch_pid = 0; if (sscanf(linebuf_ptr, "%u", &watch_pid) < 1) continue; @@ -330,16 +323,11 @@ static bool opts_watchfd_listfile(opts_t opts, const char *filename) size_t linebuf_size = 0; unsigned int linenumber; - /* - * flawfinder: buffer is zeroed before each use, and fgets() is - * passed one less than its size so the string functions in the loop - * always find a null byte at the end. - */ - fptr = fopen(filename, "r"); /* flawfinder: ignore */ /* - * flawfinder note: caller directly controls filename, and we're - * opening the file read-only. + * flawfinder note: the caller directly controls the filename, which + * here is being opened read-only, so there is no further + * mitigation. */ if (NULL == fptr) { fprintf(stderr, "%s: -d @: %s: %s\n", opts->program_name, filename, strerror(errno)); @@ -355,7 +343,7 @@ static bool opts_watchfd_listfile(opts_t opts, const char *filename) /* Set errno to 0 to distinguish between EOF and error. */ errno = 0; - /*@-unrecog@ *//* splint dosn't know of getline(). */ + /*@-unrecog@ *//* splint doesn't know of getline(). */ line_length = getline(&linebuf_ptr, &linebuf_size, fptr); /*@+unrecog@ */ if ((line_length < 0) || (NULL == linebuf_ptr)) { @@ -626,7 +614,7 @@ static bool opts_use_size_of_file(opts_t opts, const char *size_file) sysfs_fptr = fopen(sysfs_filename, "r"); /* flawfinder: ignore */ /* * flawfinder rationale: sysfs is trusted here, the filename is - * predictable, and we are restricted to reading one number. + * predictable, and only one number is being read. */ if (NULL != sysfs_fptr) { sysfs_size = -1; @@ -637,7 +625,7 @@ static bool opts_use_size_of_file(opts_t opts, const char *size_file) return true; } /* Read not successful - report the error and return. */ - /* NB we must fclose() after reporting to retain errno. */ + /* NB fclose() comes after the error report, to retain errno. */ /*@-mustfreefresh@ *//* see above */ fprintf(stderr, "%s: %s: %s: %s\n", opts->program_name, size_file, _("failed to read sysfs size file"), strerror(errno)); @@ -653,8 +641,8 @@ static bool opts_use_size_of_file(opts_t opts, const char *size_file) device_fd = open(size_file, O_RDONLY); /* flawfinder: ignore */ /* * flawfinder rationale: the filename is under the direct control of - * the operator by its nature, so we can't refuse to open symlinks - * etc as that would be counterintuitive. + * the operator by its nature, so no further mitigation is possible. + * For example, refusing to open symlinks would be counterintuitive. */ if (device_fd < 0) { @@ -679,7 +667,7 @@ static bool opts_use_size_of_file(opts_t opts, const char *size_file) (void) close(device_fd); - /* Use the size we found. */ + /* Set the size to the detected value. */ opts->size = device_size; return true; } @@ -818,10 +806,10 @@ opts_t opts_parse(unsigned int argc, char **argv) c = getopt((int) argc, argv, short_options); /* flawfinder: ignore */ #endif /* - * flawfinder rationale: we have to pass argv to getopt, and - * limiting the argument sizes would be impractical and - * cumbersome (and likely lead to more bugs); so we have to - * trust the system getopt to not have internal buffer + * flawfinder rationale: argv has to be passed to getopt, + * and limiting the argument sizes would be impractical and + * cumbersome (and likely lead to more bugs); so the system + * getopt has to be trusted not to have internal buffer * overflows. */ @@ -888,11 +876,11 @@ opts_t opts_parse(unsigned int argc, char **argv) /*@+mustfreefresh@ */ } else if (0 != access(optarg + 1, R_OK)) { /* flawfinder: ignore */ /* - * flawfinder rationale: we're not - * using access() to check - * permissions, only to help give a - * usable error message, so there's - * no TOCTOU issue. + * flawfinder rationale: there is no + * TOCTOU issue because access() is + * only being used to inform the + * error message, not to check + * permissions. */ /*@-mustfreefresh@ *//* see above */ fprintf(stderr, "%s: -%c @: %s: %s\n", @@ -946,11 +934,11 @@ opts_t opts_parse(unsigned int argc, char **argv) case 'h': display_help(); opts->action = PV_ACTION_NOTHING; - return opts; /* early return */ + return opts; /* Early return. */ case 'V': display_version(); opts->action = PV_ACTION_NOTHING; - return opts; /* early return */ + return opts; /* Early return. */ case 'p': opts->progress = true; numopts++; @@ -1221,8 +1209,9 @@ opts_t opts_parse(unsigned int argc, char **argv) } while (c != -1); /* - * splint thinks we can reach here after opts_free() and opts=NULL - * above, so explicitly return here if opts was set to NULL. + * splint thinks this point is reachable after opts_free() and + * opts=NULL above, so explicitly return here if opts was set to + * NULL. */ if (NULL == opts) return NULL; @@ -1323,7 +1312,7 @@ opts_t opts_parse(unsigned int argc, char **argv) opts->bytes = true; } - /* If -Z was given but not -E, pretend one -E was given too. */ + /* If -Z was given but not -E, behave as if one -E was given too. */ if (opts->error_skip_block > 0 && 0 == opts->skip_errors) opts->skip_errors = 1; diff --git a/src/main/version.c b/src/main/version.c index 5816966..aed1e8b 100644 --- a/src/main/version.c +++ b/src/main/version.c @@ -18,16 +18,16 @@ void display_version(void) /* * splint note: the gettext calls made by _() cause memory leak * warnings, but in this case it's unavoidable, and mitigated by the - * fact we only translate each string once. + * fact that each string is only translated once. */ - /* GNU standard first line format: program and version only */ + /* GNU standard first line format: program and version only. */ printf("%s %s\n", PACKAGE_NAME, PACKAGE_VERSION); - /* GNU standard second line format - "Copyright" always in English */ + /* GNU standard second line format - "Copyright" always in English. */ printf("Copyright %s %s\n", "2026", "Andrew Wood"); - /* GNU standard license line and free software notice */ + /* GNU standard license line and free software notice. */ printf("%s\n", _("License: GPLv3+ ")); printf("%s\n", _("This is free software: you are free to change and redistribute it.")); printf("%s\n", _("There is NO WARRANTY, to the extent permitted by law.")); - /* Project web site link */ + /* Project web site link. */ printf("\n%s: <%s>\n", _("Project web site"), PACKAGE_URL); } diff --git a/src/pv/calc.c b/src/pv/calc.c index a03200b..0567609 100644 --- a/src/pv/calc.c +++ b/src/pv/calc.c @@ -103,8 +103,8 @@ void pv_calculate_transfer_rate(pvtransfercalc_t calc, readonly_pvtransferstate_ } /* - * In case the time since the last update is very small, we keep - * track of amount transferred since the last update, and just keep + * When the time since the last update is very small, keep track of + * the amount transferred since the last update, and just keep * adding to that until a reasonable amount of time has passed to * avoid rate spikes or division by zero. */ @@ -133,7 +133,7 @@ void pv_calculate_transfer_rate(pvtransfercalc_t calc, readonly_pvtransferstate_ } calc->prev_rate = transfer_rate; - /* Update history and current average rate for ETA. */ + /* Update the history and current average rate. */ pv__update_average_rate_history(calc, transfer, control->history_interval, transfer_rate); average_rate = calc->current_avg_rate; @@ -158,12 +158,11 @@ void pv_calculate_transfer_rate(pvtransfercalc_t calc, readonly_pvtransferstate_ if (control->size <= 0) { /* - * If we don't know the total size of the incoming data, - * then for a percentage, we gradually increase the - * percentage completion as data arrives, to a maximum of - * 200, then reset it - we use this if we can't calculate - * it, so that the numeric percentage output will go - * 0%-100%, 100%-0%, 0%-100%, and so on. + * If the total size of the incoming data is unknown, then + * for a percentage, gradually increase the percentage + * completion as data arrives, to a maximum of 200, then + * reset it. This means that the numeric percentage output + * will go 0%-100%, 100%-0%, 0%-100%, and so on. */ if (transfer_rate > 0) calc->percentage += 2; diff --git a/src/pv/cursor.c b/src/pv/cursor.c index c49a029..6e5c5ef 100644 --- a/src/pv/cursor.c +++ b/src/pv/cursor.c @@ -7,8 +7,8 @@ * first `pv' process. * * However, some OSes (FreeBSD and MacOS X so far) don't allow locking of a - * terminal, so we try to use a lockfile if terminal locking doesn't work, - * and finally abort if even that is unavailable. + * terminal - so if terminal locking doesn't work, a separate lock file is + * used, and cursor positioning is abandoned if that also fails. * * Copyright 2002-2008, 2010, 2012-2015, 2017, 2021, 2023-2026 Andrew Wood * @@ -44,7 +44,7 @@ /* - * Create a per-euid, per-tty, lockfile in ${TMPDIR:-${TMP:-/tmp}} for the + * Create a per-euid, per-tty, lock file in ${TMPDIR:-${TMP:-/tmp}} for the * tty on the given file descriptor. */ static void pv_crs_open_lockfile(pvcursorstate_t cursor, readonly_pvcontrol_t control, int fd) @@ -68,8 +68,8 @@ static void pv_crs_open_lockfile(pvcursorstate_t cursor, readonly_pvcontrol_t co pv_error("%s: %s", _("failed to get terminal name"), strerror(errno)); } /* - * If we don't know our terminal name, we can neither do IPC - * nor make a lock file, so turn off cursor positioning. + * If the terminal name is unknown, then neither IPC nor a + * lock file are feasible, so turn off cursor positioning. */ cursor->disable = true; debug("%s", "ttyname failed - cursor positioning disabled"); @@ -103,10 +103,10 @@ static void pv_crs_open_lockfile(pvcursorstate_t cursor, readonly_pvcontrol_t co cursor->lock_fd = open(cursor->lock_file, openflags, 0600); /* flawfinder: ignore */ /* - * flawfinder rationale: we aren't truncating the lock file, we - * don't change its contents, and we are attempting to use - * O_NOFOLLOW where possible to avoid symlink attacks, so this - * open() is as safe as we can make it. + * flawfinder rationale: the file isn't being truncated and its + * contents won't be changed, and O_NOFOLLOW will be used if + * available (to avoid symlink attacks), so this open() has been made + * as safe as possible. */ if (cursor->lock_fd < 0) { @@ -119,7 +119,7 @@ static void pv_crs_open_lockfile(pvcursorstate_t cursor, readonly_pvcontrol_t co /* * Lock the terminal on the given file descriptor, falling back to using a - * lockfile if the terminal itself cannot be locked. + * lock file if the terminal itself cannot be locked. */ static void pv_crs_lock(pvcursorstate_t cursor, readonly_pvcontrol_t control, int fd) { @@ -150,7 +150,7 @@ static void pv_crs_lock(pvcursorstate_t cursor, readonly_pvcontrol_t control, in } if (cursor->lock_fd >= 0) { - debug("%s: %s", cursor->lock_file, "terminal lockfile acquired"); + debug("%s: %s", cursor->lock_file, "terminal lock file acquired"); } else { debug("%s", "terminal lock acquired"); } @@ -159,7 +159,7 @@ static void pv_crs_lock(pvcursorstate_t cursor, readonly_pvcontrol_t control, in /* * Unlock the terminal on the given file descriptor. If pv_crs_lock used - * lockfile locking, unlock the lockfile. + * lock file locking, unlock the lock file. */ static void pv_crs_unlock(pvcursorstate_t cursor, int fd) { @@ -178,7 +178,7 @@ static void pv_crs_unlock(pvcursorstate_t cursor, int fd) (void) fcntl(lock_fd, F_SETLK, &lock); if (cursor->lock_fd >= 0) { - debug("%s: %s", cursor->lock_file, "terminal lockfile released"); + debug("%s: %s", cursor->lock_file, "terminal lock file released"); } else { debug("%s", "terminal lock released"); } @@ -189,8 +189,8 @@ static void pv_crs_unlock(pvcursorstate_t cursor, int fd) /* * Get the current number of processes attached to our shared memory * segment, i.e. find out how many `pv' processes in total are running in - * cursor mode (including us), and store it in pv_crs_pvcount. If this is - * larger than pv_crs_pvmax, update pv_crs_pvmax. + * cursor mode (including this process), and store it in pv_crs_pvcount. If + * this is larger than pv_crs_pvmax, update pv_crs_pvmax. */ static void pv_crs_ipccount(pvcursorstate_t cursor) { @@ -246,7 +246,7 @@ static int pv_crs_get_ypos(int terminalfd) memset(cpr, 0, sizeof(cpr)); #ifdef CURSOR_ANSWERBACK_BYTE_BY_BYTE - /* Read answerback byte by byte - fails on AIX */ + /* Read answerback byte by byte - fails on AIX. */ for (got = 0, r = 0; got < (int) (sizeof(cpr) - 2); got += r) { r = read(terminalfd, cpr + got, 1); /* flawfinder: ignore */ /* flawfinder rationale: bounded to buffer size by "for" */ @@ -263,9 +263,9 @@ static int pv_crs_get_ypos(int terminalfd) terminalfd, got, cpr[0], cpr[1], cpr[2], cpr[3], cpr[4], cpr[5]); #else /* !CURSOR_ANSWERBACK_BYTE_BY_BYTE */ - /* Read answerback in one big lump - may fail on Solaris */ + /* Read answerback in one big lump - may fail on Solaris. */ r = read(terminalfd, cpr, sizeof(cpr) - 2); /* flawfinder: ignore */ - /* flawfinder rationale: bounded to buffer size */ + /* flawfinder rationale: bounded to buffer size. */ if (r <= 0) { debug("r=%d: %s", r, strerror(errno)); } else { @@ -292,23 +292,24 @@ static int pv_crs_get_ypos(int terminalfd) /* * Initialise the IPC data, returning nonzero on error. * - * To do this, we attach to the shared memory segment (creating it if it - * does not exist). If we are the only process attached to it, then we - * initialise it with the current cursor position. + * Attaches to the shared memory segment (creating it if it does not exist). + * If this is the only process attached to it, then the memory is + * initialised with the current cursor position. * - * There is a race condition here: another process could attach before we've - * had a chance to check, such that no process ends up getting an "attach - * count" of one, and so no initialisation occurs. So, we lock the terminal - * with pv_crs_lock() while we are attaching and checking. + * To avoid a race condition, this function locks the terminal with + * pv_crs_lock() while attaching and checking. The race condition is that + * another process could attach before this one checks, such that no process + * ends up getting an "attach count" of one, and so no initialisation + * occurs. */ static int pv_crs_ipcinit(pvcursorstate_t cursor, readonly_pvcontrol_t control, char *ttyfile, int terminalfd) { key_t key; /* - * Base the key for the shared memory segment on our current tty, so - * we don't end up interfering in any way with instances of `pv' - * running on another terminal. + * Base the key for the shared memory segment on the current tty, to + * avoid interfering in any way with instances of `pv' running on + * another terminal. */ key = ftok(ttyfile, (int) 'p'); if (-1 == key) { @@ -330,15 +331,15 @@ static int pv_crs_ipcinit(pvcursorstate_t cursor, readonly_pvcontrol_t control, } /*@-nullpass@ */ - /* splint doesn't know shmaddr can be NULL */ + /* splint doesn't know shmaddr can be NULL. */ cursor->shared = shmat(cursor->shmid, NULL, 0); /*@+nullpass@ */ pv_crs_ipccount(cursor); /* - * If nobody else is attached to the shared memory segment, we're - * the first, so we need to initialise the shared memory with our + * If no other process is attached to the shared memory segment, + * this one is the first, so initialise the shared memory with the * current Y cursor co-ordinate and with an initial false value for * the TOSTOP-added flag. */ @@ -355,8 +356,8 @@ static int pv_crs_ipcinit(pvcursorstate_t cursor, readonly_pvcontrol_t control, cursor->y_offset = 0; /* - * If anyone else had attached to the shared memory segment, we need - * to read the top Y co-ordinate from it. + * If any other process had attached to the shared memory segment + * already, read the top Y co-ordinate from it. */ if (cursor->pvcount > 1) { cursor->y_start = cursor->shared->y_topmost; @@ -397,10 +398,10 @@ void pv_crs_init(pvcursorstate_t cursor, readonly_pvcontrol_t control, pvtransie terminalfd = open(ttyfile, O_RDWR); /* flawfinder: ignore */ /* - * flawfinder rationale: the file we open won't be truncated but - * could be corrupted by writes attempting to get the current Y - * position; but we get the filename from ttyname() and it could be - * a symbolic link, so we can't do much more than trust it. + * flawfinder rationale: the file won't be truncated but could be + * corrupted by writes attempting to get the current Y position; but + * the filename is from ttyname() and it could be a symbolic link, + * so O_NOFOLLOW isn't feasible and it has to be trusted as-is. */ if (terminalfd < 0) { @@ -415,8 +416,8 @@ void pv_crs_init(pvcursorstate_t cursor, readonly_pvcontrol_t control, pvtransie } /* - * If we have already set the terminal TOSTOP attribute, set the - * flag in shared memory to let the other instances know. + * If the terminal TOSTOP attribute was already set by this process, + * set the flag in shared memory to let the other instances know. */ if ((!cursor->noipc) && (1 == flags->clear_tty_tostop_on_exit) && (NULL != cursor->shared)) { debug("%s", "propagating local clear_tty_tostop_on_exit true value to shared tty_tostop_added flag"); @@ -424,9 +425,9 @@ void pv_crs_init(pvcursorstate_t cursor, readonly_pvcontrol_t control, pvtransie } /* - * If we are not using IPC, then we need to get the current Y - * co-ordinate. If we are using IPC, then the pv_crs_ipcinit() - * function takes care of this in a more multi-process-friendly way. + * If IPC is not being used, the current Y needs to be determined. + * If IPC is being used, then the pv_crs_ipcinit() function takes + * care of this in a more multi-process-friendly way. */ if (cursor->noipc) { #else /* ! HAVE_IPC */ @@ -456,7 +457,7 @@ void pv_crs_init(pvcursorstate_t cursor, readonly_pvcontrol_t control, pvtransie #ifdef HAVE_IPC /* - * Set the "we need to reinitialise cursor positioning" flag. + * Set the "need to reinitialise cursor positioning" flag. */ void pv_crs_needreinit(pvcursorstate_t cursor) { @@ -469,8 +470,8 @@ void pv_crs_needreinit(pvcursorstate_t cursor) #ifdef HAVE_IPC /* - * Reinitialise the cursor positioning code (called if we are backgrounded - * then foregrounded again). + * Reinitialise the cursor positioning code (called if the process is + * backgrounded then foregrounded again). */ static void pv_crs_reinit(pvcursorstate_t cursor, readonly_pvcontrol_t control, pvtransientflags_t flags) { @@ -555,8 +556,8 @@ void pv_crs_update(pvcursorstate_t cursor, readonly_pvcontrol_t control, pvtrans /* * If the screen has scrolled, or is about to scroll, due to * multiple `pv' instances taking us near the bottom of the screen, - * scroll the screen (only if we're the first `pv'), and then move - * our initial Y co-ordinate up. + * scroll the screen (only if this is the first `pv'), and then move + * the initial Y co-ordinate up. */ if (((cursor->y_start + cursor->pvmax) > (int) (control->height)) && (!cursor->noipc) @@ -572,7 +573,7 @@ void pv_crs_update(pvcursorstate_t cursor, readonly_pvcontrol_t control, pvtrans debug("%s: %d", "scroll offset", offs); /* - * Scroll the screen if we're the first `pv'. + * Scroll the screen if this is the first `pv'. */ if (0 == cursor->y_offset) { pv_crs_lock(cursor, control, STDERR_FILENO); @@ -596,8 +597,8 @@ void pv_crs_update(pvcursorstate_t cursor, readonly_pvcontrol_t control, pvtrans #endif /* HAVE_IPC */ /* - * Keep the Y co-ordinate within sensible bounds, so we can never - * overflow the "cup_cmd" buffer. + * Clamp the Y co-ordinate to sensible bounds, to avoid overflowing + * the "cup_cmd" buffer. */ if ((y < 1) || (y > 999999)) y = 1; @@ -630,7 +631,7 @@ void pv_crs_fini(pvcursorstate_t cursor, readonly_pvcontrol_t control, pvtransie char cup_cmd[32]; /* flawfinder: ignore */ unsigned int y; - /* flawfinder - "cup_cmd" is zeroed, and only written by pv_snprintf() */ + /* flawfinder - "cup_cmd" is zeroed, and only written by pv_snprintf(). */ debug("%s", "fini"); @@ -645,7 +646,7 @@ void pv_crs_fini(pvcursorstate_t cursor, readonly_pvcontrol_t control, pvtransie y = control->height; /* - * Absolute bounds check. + * Clamp to reasonable values. */ if ((y < 1) || (y > 999999)) y = 1; @@ -663,7 +664,7 @@ void pv_crs_fini(pvcursorstate_t cursor, readonly_pvcontrol_t control, pvtransie #ifdef HAVE_IPC /* * If any other "pv -c" instances have set the terminal TOSTOP - * attribute, set our local flag so pv_sig_fini() will know about + * attribute, set the local flag so pv_sig_fini() will know about * it. */ if ((!cursor->noipc) && (NULL != cursor->shared) && cursor->shared->tty_tostop_added) { @@ -681,7 +682,7 @@ void pv_crs_fini(pvcursorstate_t cursor, readonly_pvcontrol_t control, pvtransie cursor->shared = NULL; /* - * If we are the last instance detaching from the shared memory, + * If this is the last instance detaching from the shared memory, * delete it so it's not left lying around. */ if (cursor->pvcount < 2) { @@ -697,8 +698,9 @@ void pv_crs_fini(pvcursorstate_t cursor, readonly_pvcontrol_t control, pvtransie if (cursor->lock_fd >= 0) { (void) close(cursor->lock_fd); /* - * We can get away with removing this on exit because all - * the other PVs will be finishing at the same sort of time. + * Since all PVs in a pipeline are likely to finish around + * the same time, the lock file can be removed without + * further co-ordination. */ (void) remove(cursor->lock_file); }