> 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 > >
