[vlc-devel] [PATCH] record: use vlc_mkstemp instead of tempnam

[Steve Lhomme]

On 2020-09-29 6:06, Lyndon Brown wrote:
> On Mon, 2020-09-28 at 07:48 +0200, Steve Lhomme wrote:
>> On 2020-09-27 22:30, Lyndon Brown wrote:
>>> +            psz_file = strdup( DIR_SEP"tmp"DIR_SEP"vlc-
>>> recording.XXXXXX" );
>>
>> I'm pretty sure /tmp doesn't exist on Windows. And it will
>> definitely
>> not be cleaned by the system when the user tells the OS to clean the
>> temporary files.
> 
> Hi :)
> 
> FWIW, just to nitpick, on Windows it wouldn't be /tmp, we'd be talking
> about "\tmp\foo" due to DIR_SEP being a backslash on Windows, which
> would be interpreted as being <current-device>\tmp\foo.
> 
> Yes indeed there's a problem with respect to Windows; This wasn't a
> failure to recognise the problem there, I've simply completely
> forgotten that when I originally made the patch last year that I
> intended to raise this question of what we should do here wrt. Windows
> when I eventually submitted it, which is coming back to me now.
> Apologies for forgetting and thanks for catching it.
> 
> The behaviour of the tempnam() function call being replaced, per the
> man page, is that it follows a set of defined rules to determine where
> to create the file. The mkstemp() function and friends don't do this I
> don't think (the man page is not perfectly clear), and certainly the
> implementation of vlc_mkstemp() in src/text/filesystem.c doesn't do any
> such thing.

I think using tmpnam() to generate a temporary name is the way to go. 
The problem here seems to be where we store temporary files. I think the 
current implementation uses the "current directory". It would be better 
to use the proper system folder to do that. That's the missing part.

On Windows there is GetTempPath which gives the proper folder to use. 
It's used in vlc_win32_tmpfile() but it gives a FILE*. IMO we should 
have a core API that gives either the folder or a file path in the 
temporary folder.

> I copied use of `DIR_SEP "tmp" DIR_SEP` from other existing use of the
> function in the codebase, though it seemed wrong in relation to
> Windows, hence having intended to ask. We have three unit tests with
> hard coded "/tmp/foo" strings, and this same `DIR_SEP "tmp" DIR_SEP`
> thing being done in the GetTmpFile() function of es_out_timeshift.c
> when it's not given a path. (There are also hard coded "/tmp/foo" paths
> in vlc_memfd() for posix/linux, but at least that's not so
> problematic).
> 
> So what is up with these existing users of vlc_mkstemp() wrt. Windows?
> Why are these tests hard coding *nix paths? Why does GetTmpFile() use a
> *nix path if not given a directory path?
> 
> And how do we approach using it correctly as a proper replacement for
> tempnam() in record and vorepository?
> 
> It would appear to me that we need to dynamically define the correct
> standard system temp path to use, either in a hard coded way or via
> some header, and then use that for our vlc_mkstemp() calls? Right?
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>