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