On Wed, Aug 31, 2011 at 07:20:42AM -0500, Marco Peereboom wrote:
> On Wed, Aug 31, 2011 at 02:13:19PM +0200, LEVAI Daniel wrote:
> > On Wed, Aug 31, 2011 at 06:57:34 -0500, Marco Peereboom wrote:
> > > On Wed, Aug 31, 2011 at 11:59:29AM +0200, LEVAI Daniel wrote:
> > > > On Tue, Aug 30, 2011 at 13:55:57 -0500, Marco Peereboom wrote:
> > > > > On Tue, Aug 30, 2011 at 11:11:46AM -0500, Marco Peereboom wrote:
> > > > > > I have had enough of corrupt ksh history so I had a look at the
> > > > > > code to
> > > > > > try to fix it. The magical code was very magical so I basically
> > > > > > deleted
> > > > > > most of it and made ksh history into a flat text file. It handles
> > > > > > multiple ksh instances writing to the same text file with locks just
> > > > > > like the current ksh does. I haven't noticed any differences in
> > > > > > behavior running this.
> > > >
> > > > If one had set HISTFILE='', the old behaviour was to not write a history
> > > > file, but the "in memory" history was still working.
> > > > With this patch, if I set HISTFILE='' then there will be no command
> > > > history at all.
> > >
> > > This is the same with current ksh.
> >
> > Not for me. Currently if I set HISTFILE to '' then in-memory history
> > works but it doesn't write to history file (which was my intention when
> > I've set HISTFILE='').
>
> Hmmm, I tried again and now I see it. I blame it on not enough coffee.
> Thanks I'll go figure it out.
Version 4 fixes all reported bugs.
Some folks have expressed doubt about the simplistic way of updating the
history file. Specifically the rewriting of all entries. I am
sensitive to that and know a couple of optimizations that can easily be
applied. However before I go there I'd like to get a thumbs up or down
on this approach. It trashes the binary history file format and
replaces it with flat text. Is this something we want?
Index: alloc.c
===================================================================
RCS file: /cvs/src/bin/ksh/alloc.c,v
retrieving revision 1.8
diff -u -p -r1.8 alloc.c
--- alloc.c 21 Jul 2008 17:30:08 -0000 1.8
+++ alloc.c 30 Aug 2011 18:05:47 -0000
@@ -62,7 +62,7 @@ alloc(size_t size, Area *ap)
{
struct link *l;
- l = malloc(sizeof(struct link) + size);
+ l = calloc(1, sizeof(struct link) + size);
if (l == NULL)
internal_errorf(1, "unable to allocate memory");
l->next = ap->freelist;
Index: history.c
===================================================================
RCS file: /cvs/src/bin/ksh/history.c,v
retrieving revision 1.39
diff -u -p -r1.39 history.c
--- history.c 19 May 2010 17:36:08 -0000 1.39
+++ history.c 31 Aug 2011 19:33:24 -0000
@@ -11,8 +11,7 @@
* a) the original in-memory history mechanism
* b) a more complicated mechanism done by p...@hillside.co.uk
* that more closely follows the real ksh way of doing
- * things. You need to have the mmap system call for this
- * to work on your system
+ * things.
*/
#include "sh.h"
@@ -22,19 +21,10 @@
# include <sys/file.h>
# include <sys/mman.h>
-/*
- * variables for handling the data file
- */
-static int histfd;
-static int hsize;
-
-static int hist_count_lines(unsigned char *, int);
-static int hist_shrink(unsigned char *, int);
-static unsigned char *hist_skip_back(unsigned char *,int *,int);
-static void histload(Source *, unsigned char *, int);
-static void histinsert(Source *, int, unsigned char *);
-static void writehistfile(int, char *);
-static int sprinkle(int);
+static void writehistfile(FILE *);
+static FILE *history_open(int *);
+static int history_load(FILE *, Source *);
+static void history_close(FILE *);
static int hist_execute(char *);
static int hist_replace(char **, const char *, const char *, int);
@@ -45,8 +35,8 @@ static void histbackup(void);
static char **current; /* current position in history[] */
static char *hname; /* current name of history file */
static int hstarted; /* set after hist_init() called */
-static Source *hist_source;
-
+static Source *hist_source;
+static struct stat last_sb;
int
c_fc(char **wp)
@@ -529,15 +519,10 @@ sethistfile(const char *name)
/* if the name is the same as the name we have */
if (hname && strcmp(hname, name) == 0)
return;
-
/*
* its a new name - possibly
*/
- if (histfd) {
- /* yes the file is open */
- (void) close(histfd);
- histfd = 0;
- hsize = 0;
+ if (hname) {
afree(hname, APERM);
hname = NULL;
/* let's reset the history */
@@ -577,18 +562,26 @@ init_histvec(void)
void
histsave(int lno, const char *cmd, int dowrite)
{
- char **hp;
- char *c, *cp;
+ char **hp;
+ char *c, *cp;
+ int changed;
+ FILE *f = NULL;
+
+ if (dowrite) {
+ f = history_open(&changed);
+ if (f && changed) {
+ /* reset history */
+ histptr = history - 1;
+ hist_source->line = 0;
+ history_load(f, hist_source);
+ }
+ }
c = str_save(cmd, APERM);
if ((cp = strchr(c, '\n')) != NULL)
*cp = '\0';
- if (histfd && dowrite)
- writehistfile(lno, c);
-
hp = histptr;
-
if (++hp >= history + histsize) { /* remove oldest command */
afree((void*)*history, APERM);
for (hp = history; hp < history + histsize - 1; hp++)
@@ -596,371 +589,125 @@ histsave(int lno, const char *cmd, int d
}
*hp = c;
histptr = hp;
-}
-
-/*
- * Write history data to a file nominated by HISTFILE
- * if HISTFILE is unset then history still happens, but
- * the data is not written to a file
- * All copies of ksh looking at the file will maintain the
- * same history. This is ksh behaviour.
- *
- * This stuff uses mmap()
- * if your system ain't got it - then you'll have to undef HISTORYFILE
- */
-/*
- * Open a history file
- * Format is:
- * Bytes 1, 2: HMAGIC - just to check that we are dealing with
- * the correct object
- * Then follows a number of stored commands
- * Each command is
- * <command byte><command number(4 bytes)><bytes><null>
- */
-#define HMAGIC1 0xab
-#define HMAGIC2 0xcd
-#define COMMAND 0xff
+ if (dowrite && f) {
+ writehistfile(f);
+ history_close(f);
+ }
+}
-void
-hist_init(Source *s)
+static FILE *
+history_open(int *changed)
{
- unsigned char *base;
- int lines;
- int fd;
-
- if (Flag(FTALKING) == 0)
- return;
-
- hstarted = 1;
-
- hist_source = s;
-
- hname = str_val(global("HISTFILE"));
- if (hname == NULL)
- return;
- hname = str_save(hname, APERM);
+ int fd;
+ FILE *f = NULL;
+ struct stat sb;
- retry:
- /* we have a file and are interactive */
- if ((fd = open(hname, O_RDWR|O_CREAT|O_APPEND, 0600)) < 0)
- return;
-
- histfd = savefd(fd);
- if (histfd != fd)
+ if ((fd = open(hname, O_RDWR | O_CREAT | O_EXLOCK, 0600)) == -1)
+ return (NULL);
+ f = fdopen(fd, "r+");
+ if (f == NULL) {
close(fd);
+ goto bad;
+ }
- (void) flock(histfd, LOCK_EX);
+ if (fstat(fileno(f), &sb) == -1)
+ goto bad;
+ if (timespeccmp(&sb.st_mtim, &last_sb.st_mtim, ==))
+ *changed = 0;
+ else
+ *changed = 1;
- hsize = lseek(histfd, 0L, SEEK_END);
+ return (f);
+bad:
+ if (f)
+ fclose(f);
- if (hsize == 0) {
- /* add magic */
- if (sprinkle(histfd)) {
- hist_finish();
- return;
- }
- }
- else if (hsize > 0) {
- /*
- * we have some data
- */
- base = (unsigned char *)mmap(0, hsize, PROT_READ,
- MAP_FILE|MAP_PRIVATE, histfd, 0);
- /*
- * check on its validity
- */
- if (base == MAP_FAILED || *base != HMAGIC1 || base[1] !=
HMAGIC2) {
- if (base != MAP_FAILED)
- munmap((caddr_t)base, hsize);
- hist_finish();
- if (unlink(hname) != 0)
- return;
- goto retry;
- }
- if (hsize > 2) {
- lines = hist_count_lines(base+2, hsize-2);
- if (lines > histsize) {
- /* we need to make the file smaller */
- if (hist_shrink(base, hsize))
- if (unlink(hname) != 0)
- return;
- munmap((caddr_t)base, hsize);
- hist_finish();
- goto retry;
- }
- }
- histload(hist_source, base+2, hsize-2);
- munmap((caddr_t)base, hsize);
- }
- (void) flock(histfd, LOCK_UN);
- hsize = lseek(histfd, 0L, SEEK_END);
+ return (NULL);
}
-typedef enum state {
- shdr, /* expecting a header */
- sline, /* looking for a null byte to end the line */
- sn1, /* bytes 1 to 4 of a line no */
- sn2, sn3, sn4
-} State;
+static void
+history_close(FILE *f)
+{
+ fflush(f);
+ fstat(fileno(f), &last_sb);
+ fclose(f);
+}
static int
-hist_count_lines(unsigned char *base, int bytes)
+history_load(FILE *f, Source *s)
{
- State state = shdr;
- int lines = 0;
+ char *p, line[LINE + 1];
+ uint32_t i;
- while (bytes--) {
- switch (state) {
- case shdr:
- if (*base == COMMAND)
- state = sn1;
+ /* just read it all; will auto resize history upon next command */
+ for (i = 1; ; i++) {
+ p = fgets(line, sizeof line, f);
+ if (p == NULL || feof(f) || ferror(f))
break;
- case sn1:
- state = sn2; break;
- case sn2:
- state = sn3; break;
- case sn3:
- state = sn4; break;
- case sn4:
- state = sline; break;
- case sline:
- if (*base == '\0')
- lines++, state = shdr;
+ if ((p = strchr(line, '\n')) == NULL) {
+ bi_errorf("history file is corrupt");
+ return (1);
}
- base++;
+ *p = '\0';
+
+ s->line = i;
+ s->cmd_offset = i;
+ histsave(i, (char *)line, 0);
}
- return lines;
+
+ return (0);
}
-/*
- * Shrink the history file to histsize lines
- */
-static int
-hist_shrink(unsigned char *oldbase, int oldbytes)
+void
+hist_init(Source *s)
{
- int fd;
- char nfile[1024];
- struct stat statb;
- unsigned char *nbase = oldbase;
- int nbytes = oldbytes;
-
- nbase = hist_skip_back(nbase, &nbytes, histsize);
- if (nbase == NULL)
- return 1;
- if (nbase == oldbase)
- return 0;
-
- /*
- * create temp file
- */
- (void) shf_snprintf(nfile, sizeof(nfile), "%s.%d", hname, procpid);
- if ((fd = open(nfile, O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0)
- return 1;
+ FILE *f = NULL;
+ int changed;
- if (sprinkle(fd)) {
- close(fd);
- unlink(nfile);
- return 1;
- }
- if (write(fd, nbase, nbytes) != nbytes) {
- close(fd);
- unlink(nfile);
- return 1;
- }
- /*
- * worry about who owns this file
- */
- if (fstat(histfd, &statb) >= 0)
- fchown(fd, statb.st_uid, statb.st_gid);
- close(fd);
+ if (Flag(FTALKING) == 0)
+ return;
- /*
- * rename
- */
- if (rename(nfile, hname) < 0)
- return 1;
- return 0;
-}
+ hstarted = 1;
+ hist_source = s;
-/*
- * find a pointer to the data `no' back from the end of the file
- * return the pointer and the number of bytes left
- */
-static unsigned char *
-hist_skip_back(unsigned char *base, int *bytes, int no)
-{
- int lines = 0;
- unsigned char *ep;
+ hname = str_val(global("HISTFILE"));
+ if (hname == NULL)
+ return;
+ hname = str_save(hname, APERM);
- for (ep = base + *bytes; --ep > base; ) {
- /* this doesn't really work: the 4 byte line number that is
- * encoded after the COMMAND byte can itself contain the
- * COMMAND byte....
- */
- for (; ep > base && *ep != COMMAND; ep--)
- ;
- if (ep == base)
- break;
- if (++lines == no) {
- *bytes = *bytes - ((char *)ep - (char *)base);
- return ep;
- }
- }
- return NULL;
-}
+ f = history_open(&changed);
+ if (f == NULL)
+ return;
-/*
- * load the history structure from the stored data
- */
-static void
-histload(Source *s, unsigned char *base, int bytes)
-{
- State state;
- int lno = 0;
- unsigned char *line = NULL;
-
- for (state = shdr; bytes-- > 0; base++) {
- switch (state) {
- case shdr:
- if (*base == COMMAND)
- state = sn1;
- break;
- case sn1:
- lno = (((*base)&0xff)<<24);
- state = sn2;
- break;
- case sn2:
- lno |= (((*base)&0xff)<<16);
- state = sn3;
- break;
- case sn3:
- lno |= (((*base)&0xff)<<8);
- state = sn4;
- break;
- case sn4:
- lno |= (*base)&0xff;
- line = base+1;
- state = sline;
- break;
- case sline:
- if (*base == '\0') {
- /* worry about line numbers */
- if (histptr >= history && lno-1 != s->line) {
- /* a replacement ? */
- histinsert(s, lno, line);
- }
- else {
- s->line = lno;
- s->cmd_offset = lno;
- histsave(lno, (char *)line, 0);
- }
- state = shdr;
- }
- }
- }
+ history_load(f, s);
+ history_close(f);
}
-/*
- * Insert a line into the history at a specified number
- */
static void
-histinsert(Source *s, int lno, unsigned char *line)
+writehistfile(FILE *f)
{
- char **hp;
+ int i;
+ char *cmd;
- if (lno >= s->line-(histptr-history) && lno <= s->line) {
- hp = &histptr[lno-s->line];
- if (*hp)
- afree((void*)*hp, APERM);
- *hp = str_save((char *)line, APERM);
- }
-}
+ if (ftruncate(fileno(f), 0) == -1)
+ return;
+ rewind(f);
-/*
- * write a command to the end of the history file
- * This *MAY* seem easy but it's also necessary to check
- * that the history file has not changed in size.
- * If it has - then some other shell has written to it
- * and we should read those commands to update our history
- */
-static void
-writehistfile(int lno, char *cmd)
-{
- int sizenow;
- unsigned char *base;
- unsigned char *new;
- int bytes;
- unsigned char hdr[5];
-
- (void) flock(histfd, LOCK_EX);
- sizenow = lseek(histfd, 0L, SEEK_END);
- if (sizenow != hsize) {
- /*
- * Things have changed
- */
- if (sizenow > hsize) {
- /* someone has added some lines */
- bytes = sizenow - hsize;
- base = (unsigned char *)mmap(0, sizenow,
- PROT_READ, MAP_FILE|MAP_PRIVATE, histfd, 0);
- if (base == MAP_FAILED)
- goto bad;
- new = base + hsize;
- if (*new != COMMAND) {
- munmap((caddr_t)base, sizenow);
- goto bad;
- }
- hist_source->line--;
- histload(hist_source, new, bytes);
- hist_source->line++;
- lno = hist_source->line;
- munmap((caddr_t)base, sizenow);
- hsize = sizenow;
- } else {
- /* it has shrunk */
- /* but to what? */
- /* we'll give up for now */
- goto bad;
- }
+ for (i = 0; i < histsize; i++) {
+ cmd = history[i];
+ if (cmd == NULL)
+ break;
+ if (fprintf(f, "%s\n", cmd) == -1)
+ return;
}
- /*
- * we can write our bit now
- */
- hdr[0] = COMMAND;
- hdr[1] = (lno>>24)&0xff;
- hdr[2] = (lno>>16)&0xff;
- hdr[3] = (lno>>8)&0xff;
- hdr[4] = lno&0xff;
- (void) write(histfd, hdr, 5);
- (void) write(histfd, cmd, strlen(cmd)+1);
- hsize = lseek(histfd, 0L, SEEK_END);
- (void) flock(histfd, LOCK_UN);
- return;
-bad:
- hist_finish();
}
void
hist_finish(void)
{
- (void) flock(histfd, LOCK_UN);
- (void) close(histfd);
- histfd = 0;
}
-
-/*
- * add magic to the history file
- */
-static int
-sprinkle(int fd)
-{
- static unsigned char mag[] = { HMAGIC1, HMAGIC2 };
-
- return(write(fd, mag, 2) != 2);
-}
-
#else /* HISTORY */
/* No history to be compiled in: dummy routines to avoid lots more ifdefs */
