[pacman-dev] [PATCH 1/2] pacman: set SA_RESTART for signal handler
Calling a signal handler interrupts some functions, most notably read() and therefore fgets(). This was less of an issue in the past because signals generally resulted in the termination of the program, the recently added SIGWINCH handler does not. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- For an easy and safe way to see the problem fixed by this and the next patch resize the terminal while pacman is waiting for a user response. src/pacman/pacman.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 12a4f7a..6787e72 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -1029,7 +1029,7 @@ int main(int argc, char *argv[]) /* Set up the structure to specify the new action. */ new_action.sa_handler = handler; sigemptyset(&new_action.sa_mask); - new_action.sa_flags = 0; + new_action.sa_flags = SA_RESTART; /* assign our handler to any signals we care about */ for(i = 0; i < sizeof(signals) / sizeof(signals[0]); i++) { -- 2.0.1
The read() underlying fgets() can be interrupted by a signal handler causing fgets() to return NULL. This was less of an issue in the past because signals generally resulted in the termination of the program, the recently added SIGWINCH handler does not. Replace all fgets calls with a wrapper that retries on EINTR. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- fgets_eintr is a horrible name, suggestions welcome... This is needed in addition to the previous patch for non-pacman front-ends not kind enough to set SA_RESTART. lib/libalpm/be_local.c | 14 +++++++------- lib/libalpm/trans.c | 2 +- lib/libalpm/util.c | 3 ++- src/common/util-common.c | 17 +++++++++++++++++ src/common/util-common.h | 3 +++ src/pacman/ini.c | 2 +- src/pacman/util.c | 8 ++++---- 7 files changed, 35 insertions(+), 14 deletions(-) diff --git a/lib/libalpm/be_local.c b/lib/libalpm/be_local.c index 9a9bdef..48bd0fd 100644 --- a/lib/libalpm/be_local.c +++ b/lib/libalpm/be_local.c @@ -573,7 +573,7 @@ char *_alpm_local_db_pkgpath(alpm_db_t *db, alpm_pkg_t *info, } #define READ_NEXT() do { \ - if(fgets(line, sizeof(line), fp) == NULL && !feof(fp)) goto error; \ + if(fgets_eintr(line, sizeof(line), fp) == NULL && !feof(fp)) goto error; \ _alpm_strip_newline(line, 0); \ } while(0) @@ -584,7 +584,7 @@ char *_alpm_local_db_pkgpath(alpm_db_t *db, alpm_pkg_t *info, #define READ_AND_STORE_ALL(f) do { \ char *linedup; \ - if(fgets(line, sizeof(line), fp) == NULL) {\ + if(fgets_eintr(line, sizeof(line), fp) == NULL) {\ if(!feof(fp)) goto error; else break; \ } \ if(_alpm_strip_newline(line, 0) == 0) break; \ @@ -593,7 +593,7 @@ char *_alpm_local_db_pkgpath(alpm_db_t *db, alpm_pkg_t *info, } while(1) /* note the while(1) and not (0) */ #define READ_AND_SPLITDEP(f) do { \ - if(fgets(line, sizeof(line), fp) == NULL) {\ + if(fgets_eintr(line, sizeof(line), fp) == NULL) {\ if(!feof(fp)) goto error; else break; \ } \ if(_alpm_strip_newline(line, 0) == 0) break; \ @@ -639,7 +639,7 @@ static int local_db_read(alpm_pkg_t *info, alpm_dbinfrq_t inforeq) } free(path); while(!feof(fp)) { - if(fgets(line, sizeof(line), fp) == NULL && !feof(fp)) { + if(fgets_eintr(line, sizeof(line), fp) == NULL && !feof(fp)) { goto error; } if(_alpm_strip_newline(line, 0) == 0) { @@ -728,13 +728,13 @@ static int local_db_read(alpm_pkg_t *info, alpm_dbinfrq_t inforeq) goto error; } free(path); - while(fgets(line, sizeof(line), fp)) { + while(fgets_eintr(line, sizeof(line), fp)) { _alpm_strip_newline(line, 0); if(strcmp(line, "%FILES%") == 0) { size_t files_count = 0, files_size = 0, len; alpm_file_t *files = NULL; - while(fgets(line, sizeof(line), fp) && + while(fgets_eintr(line, sizeof(line), fp) && (len = _alpm_strip_newline(line, 0))) { if(!_alpm_greedy_grow((void **)&files, &files_size, (files_size ? files_size + sizeof(alpm_file_t) : 8 * sizeof(alpm_file_t)))) { @@ -754,7 +754,7 @@ static int local_db_read(alpm_pkg_t *info, alpm_dbinfrq_t inforeq) info->files.count = files_count; info->files.files = files; } else if(strcmp(line, "%BACKUP%") == 0) { - while(fgets(line, sizeof(line), fp) && _alpm_strip_newline(line, 0)) { + while(fgets_eintr(line, sizeof(line), fp) && _alpm_strip_newline(line, 0)) { alpm_backup_t *backup; CALLOC(backup, 1, sizeof(alpm_backup_t), goto error); if(_alpm_split_backup(line, &backup)) { diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index e5328c5..6cb7eff 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -275,7 +275,7 @@ static int grep(const char *fn, const char *needle) } while(!feof(fp)) { char line[1024]; - if(fgets(line, sizeof(line), fp) == NULL) { + if(fgets_eintr(line, sizeof(line), fp) == NULL) { continue; } /* TODO: this will not work if the search string diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 6dab0de..c58c5de 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -576,8 +576,9 @@ int _alpm_run_chroot(alpm_handle_t *handle, const char *cmd, char *const argv[]) .type = ALPM_EVENT_SCRIPTLET_INFO, .line = line }; - if(fgets(line, PATH_MAX, pipe_file) == NULL) + if(fgets_eintr(line, PATH_MAX, pipe_file) == NULL) { break; + } alpm_logaction(handle, "ALPM-SCRIPTLET", "%s", line); EVENT(handle, &event); } diff --git a/src/common/util-common.c b/src/common/util-common.c index 3316eae..96dc463 100644 --- a/src/common/util-common.c +++ b/src/common/util-common.c @@ -17,6 +17,7 @@ * along with this program. If not, see <http://www.gnu.org/licenses/>. */ +#include <errno.h> #include <stdlib.h> #include <string.h> @@ -102,6 +103,22 @@ int llstat(char *path, struct stat *buf) return ret; } +/** Wrapper around fgets() which properly handles EINTR + * @param s string to read into + * @param size maximum length to read + * @param stream stream to read from + * @return value returned by fgets() + */ +char *fgets_eintr(char *s, int size, FILE *stream) +{ + char *ret; + do { + errno = 0; + ret = fgets(s, size, stream); + } while(ret == NULL && !feof(stream) && errno == EINTR); + return ret; +} + #ifndef HAVE_STRNDUP /* A quick and dirty implementation derived from glibc */ /** Determines the length of a fixed-size string. diff --git a/src/common/util-common.h b/src/common/util-common.h index 576702f..2ad23ba 100644 --- a/src/common/util-common.h +++ b/src/common/util-common.h @@ -20,6 +20,7 @@ #ifndef _PM_UTIL_COMMON_H #define _PM_UTIL_COMMON_H +#include <stdio.h> #include <sys/stat.h> /* struct stat */ const char *mbasename(const char *path); @@ -27,6 +28,8 @@ char *mdirname(const char *path); int llstat(char *path, struct stat *buf); +char *fgets_eintr(char *s, int size, FILE *stream); + #ifndef HAVE_STRNDUP char *strndup(const char *s, size_t n); #endif diff --git a/src/pacman/ini.c b/src/pacman/ini.c index 2a3ef0e..df27272 100644 --- a/src/pacman/ini.c +++ b/src/pacman/ini.c @@ -66,7 +66,7 @@ static int _parse_ini(const char *file, ini_parser_fn cb, void *data, goto cleanup; } - while(fgets(line, PATH_MAX, fp)) { + while(fgets_eintr(line, PATH_MAX, fp)) { char *key, *value, *ptr; size_t line_len; diff --git a/src/pacman/util.c b/src/pacman/util.c index 6a095fd..f665b16 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -1403,7 +1403,7 @@ int multiselect_question(char *array, int count) flush_term_input(fileno(stdin)); - if(fgets(response, response_len, stdin)) { + if(fgets_eintr(response, response_len, stdin)) { const size_t response_incr = 64; size_t len; /* handle buffer not being large enough to read full line case */ @@ -1416,7 +1416,7 @@ int multiselect_question(char *array, int count) lastchar = response + response_len - 1; /* sentinel byte */ *lastchar = 1; - if(fgets(response + response_len - response_incr - 1, + if(fgets_eintr(response + response_len - response_incr - 1, response_incr + 1, stdin) == 0) { free(response); return -1; @@ -1467,7 +1467,7 @@ int select_question(int count) flush_term_input(fileno(stdin)); - if(fgets(response, sizeof(response), stdin)) { + if(fgets_eintr(response, sizeof(response), stdin)) { size_t len = strtrim(response); if(len > 0) { int n; @@ -1521,7 +1521,7 @@ static int question(short preset, const char *format, va_list args) flush_term_input(fd_in); - if(fgets(response, sizeof(response), stdin)) { + if(fgets_eintr(response, sizeof(response), stdin)) { size_t len = strtrim(response); if(len == 0) { return preset; -- 2.0.1
On 07/08/14 07:13, Andrew Gregory wrote:
The read() underlying fgets() can be interrupted by a signal handler causing fgets() to return NULL. This was less of an issue in the past because signals generally resulted in the termination of the program, the recently added SIGWINCH handler does not. Replace all fgets calls with a wrapper that retries on EINTR.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---
fgets_eintr is a horrible name, suggestions welcome...
refgets?
participants (2)
-
Allan McRae
-
Andrew Gregory