Changeset View
Standalone View
bin/sh/histedit.c
Show All 35 Lines | |||||||||||
#endif | #endif | ||||||||||
#endif /* not lint */ | #endif /* not lint */ | ||||||||||
#include <sys/cdefs.h> | #include <sys/cdefs.h> | ||||||||||
__FBSDID("$FreeBSD$"); | __FBSDID("$FreeBSD$"); | ||||||||||
#include <sys/param.h> | #include <sys/param.h> | ||||||||||
#include <sys/stat.h> | #include <sys/stat.h> | ||||||||||
#include <dirent.h> | #include <dirent.h> | ||||||||||
#include <fcntl.h> | |||||||||||
#include <limits.h> | #include <limits.h> | ||||||||||
#include <paths.h> | #include <paths.h> | ||||||||||
#include <stdio.h> | #include <stdio.h> | ||||||||||
#include <stdlib.h> | #include <stdlib.h> | ||||||||||
#include <unistd.h> | #include <unistd.h> | ||||||||||
/* | /* | ||||||||||
* Editline and history functions (and glue). | * Editline and history functions (and glue). | ||||||||||
*/ | */ | ||||||||||
Show All 20 Lines | |||||||||||
static FILE *el_in, *el_out; | static FILE *el_in, *el_out; | ||||||||||
static char *fc_replace(const char *, char *, char *); | static char *fc_replace(const char *, char *, char *); | ||||||||||
static int not_fcnumber(const char *); | static int not_fcnumber(const char *); | ||||||||||
static int str_to_event(const char *, int); | static int str_to_event(const char *, int); | ||||||||||
static int comparator(const void *, const void *, void *); | static int comparator(const void *, const void *, void *); | ||||||||||
static char **sh_matches(const char *, int, int); | static char **sh_matches(const char *, int, int); | ||||||||||
static unsigned char sh_complete(EditLine *, int); | static unsigned char sh_complete(EditLine *, int); | ||||||||||
static const char * | |||||||||||
get_histfile(void) | |||||||||||
{ | |||||||||||
const char *histfile; | |||||||||||
/* don't try to save if the history size is 0 */ | |||||||||||
jilles: For the benefit of people who copy this code and use it in untrusted directories, it'd be… | |||||||||||
if (hist == NULL || histsizeval() == 0) | |||||||||||
return (NULL); | |||||||||||
histfile = expandstr("${HISTFILE-${HOME-}/.sh_history}"); | |||||||||||
if (histfile[0] == '\0') | |||||||||||
Done Inline Actions
pstef: | |||||||||||
Done Inline ActionsSorry, not woken up fully yet. pstef: Sorry, not woken up fully yet.
`if (hist == NULL || histsizeval() == 0)` | |||||||||||
Not Done Inline ActionsIt's probably safe to rewrite strlen(histfile) == 0 as histfile[0] == '\0' pstef: It's probably safe to rewrite `strlen(histfile) == 0` as `histfile[0] == '\0'` | |||||||||||
return (NULL); | |||||||||||
Done Inline ActionsThis unnecessarily activates a lot of machinery. A variable can be accessed without going through the shell parser and expansions, using lookupvar() or bltinlookup(). Compared to lookupvar(), bltinlookup() will ensure that a variable assignment before a builtin applies. Alternatively, do something like home = expandstr("${HOME-}/.sh_history.XXXXXXXXXX"); to put the machinery to use, and reduce code below. The return value from expandstr() is an "ungrabbed" stack string, so it will be reused automatically later and it is OK to have mkostemp() or a similar call overwrite it. The - in ${HOME-} is indeed required to avoid an error when HOME is not set and set -u is in effect. jilles: This unnecessarily activates a lot of machinery. A variable can be accessed without going… | |||||||||||
return (histfile); | |||||||||||
Done Inline ActionsPerhaps extract this line to a function so it is not duplicated. jilles: Perhaps extract this line to a function so it is not duplicated. | |||||||||||
} | |||||||||||
Done Inline ActionsEven though it requires additional lines of code, the asprintf call should be under INTOFF protection since longjmp'ing out of it is not safe. jilles: Even though it requires additional lines of code, the `asprintf` call should be under `INTOFF`… | |||||||||||
Done Inline ActionsThere should be INTOFF/INTON bracketing somewhere so that we do not leak file descriptors and memory or corrupt stdio state if a SIGINT comes in. jilles: There should be `INTOFF`/`INTON` bracketing somewhere so that we do not leak file descriptors… | |||||||||||
void | |||||||||||
histsave(void) | |||||||||||
Done Inline Actions5 is the suffix length, but the suffix means a constant character string and not the Xs. To quote the manual page, The template should be of the form /tmp/tmpXXXXXXsuffix. There is no mkostempat so I suggest leaving histfile as it is now and just passing slen of 0 here. I would also put spaces around the or operator. pstef: 5 is the suffix length, but the suffix means a constant character string and not the Xs. To… | |||||||||||
{ | |||||||||||
HistEvent he; | |||||||||||
char *histtmpname = NULL; | |||||||||||
const char *histfile; | |||||||||||
int fd; | |||||||||||
FILE *f; | |||||||||||
Done Inline ActionsI think this is backwards; I did H_FIRST here and H_PREV in the line below and it worked well for me. pstef: I think this is backwards; I did H_FIRST here and H_PREV in the line below and it worked well… | |||||||||||
Done Inline Actions
Sorry, I meant H_LAST. pstef: > I think this is backwards; I did H_FIRST here and H_PREV in the line below and it worked well… | |||||||||||
if ((histfile = get_histfile()) == NULL) | |||||||||||
return; | |||||||||||
Done Inline ActionsSuggest fdopen() and fwrite() to cut down on system calls. Might also consider a huge writev() but I think stdio is more conventional. jilles: Suggest `fdopen()` and `fwrite()` to cut down on system calls.
Might also consider a huge… | |||||||||||
INTOFF; | |||||||||||
asprintf(&histtmpname, "%s.XXXXXXXXXX", histfile); | |||||||||||
Done Inline Actions
pstef: | |||||||||||
if (histtmpname == NULL) { | |||||||||||
INTON; | |||||||||||
return; | |||||||||||
Done Inline Actionssizeof(char) has bikeshedding potential, so I suggest using 1 to avoid it pstef: sizeof(char) has bikeshedding potential, so I suggest using 1 to avoid it | |||||||||||
} | |||||||||||
fd = mkstemp(histtmpname); | |||||||||||
if (fd == -1 || (f = fdopen(fd, "w")) == NULL) { | |||||||||||
free(histtmpname); | |||||||||||
INTON; | |||||||||||
return; | |||||||||||
} | |||||||||||
if (history(hist, &he, H_SAVE_FP, f) < 1 || | |||||||||||
rename(histtmpname, histfile) == -1) | |||||||||||
unlink(histtmpname); | |||||||||||
fclose(f); | |||||||||||
free(histtmpname); | |||||||||||
INTON; | |||||||||||
} | |||||||||||
void | |||||||||||
histload(void) | |||||||||||
{ | |||||||||||
const char *histfile; | |||||||||||
HistEvent he; | |||||||||||
if ((histfile = get_histfile()) == NULL) | |||||||||||
return; | |||||||||||
Done Inline Actionslookupvar and null check here as well. jilles: `lookupvar` and null check here as well. | |||||||||||
history(hist, &he, H_LOAD, histfile); | |||||||||||
} | |||||||||||
/* | /* | ||||||||||
* Set history and editing status. Called whenever the status may | * Set history and editing status. Called whenever the status may | ||||||||||
* have changed (figures out what to do). | * have changed (figures out what to do). | ||||||||||
*/ | */ | ||||||||||
Done Inline ActionsI saw a weird issue where some commands got pushed into a single line. Removing this and the next line helped. pstef: I saw a weird issue where some commands got pushed into a single line. Removing this and the… | |||||||||||
void | void | ||||||||||
Done Inline Actions
spacing jilles: spacing | |||||||||||
histedit(void) | histedit(void) | ||||||||||
{ | { | ||||||||||
#define editing (Eflag || Vflag) | #define editing (Eflag || Vflag) | ||||||||||
if (iflag) { | if (iflag) { | ||||||||||
if (!hist) { | if (!hist) { | ||||||||||
/* | /* | ||||||||||
* turn history on | * turn history on | ||||||||||
*/ | */ | ||||||||||
INTOFF; | INTOFF; | ||||||||||
hist = history_init(); | hist = history_init(); | ||||||||||
INTON; | INTON; | ||||||||||
if (hist != NULL) | if (hist != NULL) | ||||||||||
sethistsize(histsizeval()); | sethistsize(histsizeval()); | ||||||||||
else | else | ||||||||||
out2fmt_flush("sh: can't initialize history\n"); | out2fmt_flush("sh: can't initialize history\n"); | ||||||||||
} | } | ||||||||||
if (editing && !el && isatty(0)) { /* && isatty(2) ??? */ | if (editing && !el && isatty(0)) { /* && isatty(2) ??? */ | ||||||||||
Done Inline ActionsThis is called just after processing argv, before reading the profile and/or ENV file. So it is not possible to configure the history file there. The right fix is probably to ensure histedit() is not called until the profile and/or ENV file have been read. jilles: This is called just after processing argv, before reading the profile and/or `ENV` file. So it… | |||||||||||
/* | /* | ||||||||||
* turn editing on | * turn editing on | ||||||||||
*/ | */ | ||||||||||
char *term; | char *term; | ||||||||||
INTOFF; | INTOFF; | ||||||||||
if (el_in == NULL) | if (el_in == NULL) | ||||||||||
el_in = fdopen(0, "r"); | el_in = fdopen(0, "r"); | ||||||||||
▲ Show 20 Lines • Show All 536 Lines • Show Last 20 Lines |
For the benefit of people who copy this code and use it in untrusted directories, it'd be better to have more X's, like 10.