mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Clemmer" <clemmer.alexan...@gmail.com>
Subject Re: Review Request 39559: Windows: Implemented `os::mkdtemp`.
Date Mon, 09 Nov 2015 18:01:51 GMT


> On Nov. 2, 2015, 8:38 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/mkdtemp.hpp, lines 34-71
> > <https://reviews.apache.org/r/39559/diff/3/?file=1112902#file1112902line34>
> >
> >     Can you please explain why we can't just use `mktemp` here?
> >     I acknowledge that the idea behind `mktemp` is to guarantee that the directory
name can't collide with an existing one, but the custom implementation you provide also doesn't
do that while generating the name, correct?
> 
> Joseph Wu wrote:
>     Windows doesn't have a temp-directory function.  Are you suggesting calling `mktemp`
and then using the file's name as the argument to `mkdir`?
> 
> Joris Van Remoortere wrote:
>     Indeed. It seems like that's what we are doing manually right now?
> 
> Alex Clemmer wrote:
>     I'm not a C programmer, so feel free to tell me if there is faulty reasoning here,
but a colleague looked at an early version of this code and told me that in some versions
of the C standard library on Windows `_mktemp` and `_mktemp_s` will replace the template `XXXXXX`
with the process ID, and append a single letter to the end, meaning that there are 26 unique
filenames possible. And, while `mktemp` and `mkstemp` seem to be used only in the test harness,
`mkdtemp` is in a few places that seem important. So, I figured, it will take me not-too-long
to just write the function myself, so why not. I looked at some other implementations (_e.g._
that of Postgres) but they were mostly quite complex due to trying to be cryptographically
secure. So I opted for my simple approach.
>     
>     I think you are right that we should retry if there is a name collision though, so
based on my read, I suggest that I make that change.
>     
>     Does this seem reasonable to you?

Ok, per the extended discussion, I've decided to leave this alone. The reasons, summarized,
are:

(1) We agree we can't use `_mktemp_s` and `_mktemp` due to the documentation explicitly saying
only 26 filenames are possible
(2) This is only used in tests, unlike `mkstemp` (which, by the way, does not suffer from
this problem), which means it is ok for this not to be a completely robust solution. (BTW,
we should also _never_ use `mktemp` on Linux either, for the same reason as above; this is
just a general problem with implementations of this routine, especially on older systems.)
(3) The existing solutions that are Apache v2 compatible are dramatically more complicated,
and not worth the code review overhead


- Alex


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39559/#review104778
-----------------------------------------------------------


On Nov. 9, 2015, 5:58 p.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39559/
> -----------------------------------------------------------
> 
> (Updated Nov. 9, 2015, 5:58 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Windows: Implemented `os::mkdtemp`.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 741639a942971e48e2dac42db238d423e61cac21

>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp fc2df6831ae2cb1a91c7a8cc92965939576e575d

>   3rdparty/libprocess/3rdparty/stout/include/stout/os/mkdtemp.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/mkdtemp.hpp PRE-CREATION

>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/mkdtemp.hpp PRE-CREATION

>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp e26df59d9b837e1f4a4b92577f0a3de4b9076cb4

>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp edf17d5ad8efbc988e909bfb8ffa5a015ecdc89d

> 
> Diff: https://reviews.apache.org/r/39559/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message