From: Omar Polo <op@omarpolo.com>
Subject: Re: simplify got_gotweb_openfile/flushfile
To: "Todd C. Miller" <millert@openbsd.org>
Cc: gameoftrees@openbsd.org
Date: Tue, 16 May 2023 21:16:24 +0200

I've committed these changes and also dropped the fsync() call.

Thanks!

On 2023/05/16 10:33:30 -0600, Todd C. Miller <millert@openbsd.org> wrote:
> On Tue, 16 May 2023 10:35:08 +0200, Omar Polo wrote:
> 
> > I'm not too sure about the usefulness of fsync() here by the way.  The
> > files obtained via got_gotweb_opentemp() are scratch and only used for
> > temporary storage of blobs and diffs.  Rewinding and truncating is
> > correct, otherwise we might output data from previous requests, but
> > flushing the changes to disk seems forcing unnecessary I/O to me.
> 
> I also find the fsync() strange since you are basically just recycling
> the temporary file.  I think you can just remove the fsync() call.
> 
> > Here's a diff with the rename and that makes it always closing the
> > file, even if a failure occurs inbetween.  fseek and ftruncate
> > shouldn't really fail however, so I'm likely being too paranoid.
> 
> That looks fine to me.
> 
> > The missing context is that patrick@ reported that he ran into
> >
> > : gotweb_process_request: ftruncate: Bad file descriptor
> >
> > got_gotweb_flushfile() (with its double close()) was my major suspect.
> > It's strange however to get EBADF ftruncate() since we should really
> > get it from fseek().
> 
> If you are not seeking past the current buffer, fseek() might not
> be touching the underlying descriptor.  I think the only reason you
> need the fseek() at all is to make sure that fclose() does not
> result in any writes to the underlying fd after it has been truncated.
> 
> OK millert@
> 
>  - todd