[vlc-devel] [PATCH v2 4/13] esout: refactor GetTmpFile()

[Lyndon Brown]

Remi, I've just been working on the refactor of es_out_timeshift using
tmpfile() as you suggested, but I've reached a point where I have some
significant concerns.

1. You suggested that we don't need the filename and thus could very
well just use tmpfile(), however this doesn't consider (or at least we
have not discussed) the current functionality of controlling the
directory that the temp timeshift file is stored in, which is simply
not possible with tmpfile().

We have an option, --input-timeshift-path, which I can now see is the
source of the optional directory that gets passed down to the
GetTmpFile() function. The purpose of this option is specifically to
allow users to control the directory used.

So are we keeping this option in which case we cannot in fact use
tmpfile(), or are you wanting that option ripped out?

If we are keeping it and thus abandoning the tmpfile() idea, the rest
of this below is completely redundant, so just ignore!

2. tmpfile() does not open the file in O_EXCL mode.

The C11 alternative tmpfile_s() does ("if supported by the OS") per the
link in the next point, but I lack certainty of whether it's going to
be available everywhere we need it.

3. It and tmpfile_s() are simply looked upon unfavourably here, which
recommends mkstemp and does not help me feel comfortable with
tmpfile():

https://wiki.sei.cmu.edu/confluence/display/c/FIO21-C.+Do+not+create+temporary+files+in+shared+directories

4. The MS documentation of tmpfile() states that it may require admin
privileges on Windows Vista. I doubt we care about that, but the
tmpfile_s() variant states the same thing only without a specific
version, making me question the accuracy of that of tmpfile(). Of
course if we're going with tmpfile_s() then we seem to certainly have a
problem. Furthermore alternate documentation sources like [1] also
suggest that elevated privileges may be needed on implementations like
Windows, without any reference to versions.

[1]: https://en.cppreference.com/w/c/io/tmpfile

Our existing solution does not suffer from this issue.

5. A significant reason for switching to tmpfile() is to ditch the
unnecessary path, however ditching the path may actually result in the
timeshift feature becoming slightly buggy on Windows. The stackoverflow
post at [2] suggests that there may be (rare) circumstances in which a
tmpfile() request fails having run into a delete-pending situation
involving still open handles to deleted files, like sometimes caused by
antivirus software.

[2]: 
https://stackoverflow.com/questions/6247148/tmpfile-on-windows-7-x64

Again, our existing solution does not suffer from this issue. Our
mkstemp implementation for Windows will try up to 256 different random
filenames before giving up. tmpfile() on the other hand as far as I can
understand will find the first random free filename and then fail if it
can't open it, thus being problematic.

6. One final trivial point - on Windows it supposedly creates the file
in the root directory. Is this really desirable?

Do I have your support in ditching this tmpfile() idea? Or do you
insist that we still want it, in which case I need to know what to do
about point #1?

Regards,
Lyndon