Re: No fsync on history file? I lost my history

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

lilydjwg wrote on Sun, 23 Sep 2018 23:25 +0800:
> On Sun, Sep 23, 2018 at 02:46:51PM +0000, Daniel Shahaf wrote:
> > fsync() is in POSIX.  I assume we can just call it, but if somebody complains
> > we'll need to use an HAVE_FSYNC guard.
> 
> I don't know how to add a HAVE_FSYNC macro to the build system, sorry.
> 

I doubt that's needed. I had a quick look and found that libapr's
apr_file_sync() call calls fsync() without a guard, implying that
fsync() exists on all platforms libapr is used on.

If we do need it, we'll add 'fsync' to the AC_CHECK_FUNCS call,
regenerate configure (using Util/preconfig), and use #ifdef HAVE_FSYNC.

> > > +++ b/Src/hist.c
> > > @@ -2933,6 +2933,9 @@ savehistfile(char *fn, int err, int writeflags)
> > >  		lasthist.text = ztrdup(start);
> > >  	    }
> > >  	}
> > > +	fflush(out); /* need to flush before fsync */
> > 
> > Isn't the fflush() on line 2927 sufficient?  (Even if it isn't, I would have
> > expected a ret>=0 guard around this call.)
> 
> It should call write(2) to write out the buffered data. Then the kernel
> can fsync the data to disk. A guard has been added.
> 

Yes, I understand that fflush(3) must be called in order to flush data
from libc's buffers into kernel buffers, which fsync(2) will later flush
to disk.  My question was whether the fflush() call being added was
redundant because of the existing call on line 2927.  It would be odd
to have two fflush() calls in a row without fwrite() between them; and
to have only one of them update lasthist.

Maybe it's all correct and it's just my lack of familiarity of hist.c
that's showing.

> > > +	if (fsync(fileno(out)) < 0 && ret >= 0)
> > > +	    ret = -1;
> > 
> > fileno() can return -1.
> 
> It shouldn't matter, fsync will return EBADF for -1. Other parts of the
> code don't check for this either, and I can't think a case when fileno
> would fail after so many successful I/O operations on it (corrupted memory?)
> 

Fair enough.

> > Shouldn't the ret>=0 check happen before the calls to fileno() and fsync()?
> 
> Yes, I've changed that.

Thanks.

Daniel