Re: [RFC][PATCH] mingw: avoid mktemp() in mkstemp() implementation
- From
René Scharfe <l.s.r@web.de>
- Date
- Jul 16, 2022, 06:30 UTC
- Message-ID
- <7da25e7e-e3c6-77a9-2b8f-7407e92605b9@web.de>
- In-Reply-To
- <xmqq1qum2uvy.fsf@gitster.g>
Am 15.07.22 um 18:38 schrieb Junio C Hamano:
Show 32 quoted lines
> Johannes Sixt <j6t@kdbg.org> writes:
>
>>> diff --git a/compat/mingw.c b/compat/mingw.c
>>> index 2607de93af..b5502997e2 100644
>>> --- a/compat/mingw.c
>>> +++ b/compat/mingw.c
>>> @@ -1059,10 +1059,7 @@ char *mingw_mktemp(char *template)
>>>
>>> int mkstemp(char *template)
>>> {
>>> - char *filename = mktemp(template);
>>> - if (!filename)
>>> - return -1;
>>> - return open(filename, O_RDWR | O_CREAT, 0600);
>>> + return git_mkstemp_mode(template, 0600);
>>> }
>>>
>>> int gettimeofday(struct timeval *tv, void *tz)
>>
>> I hate such an obvious layering violation. But we have already a ton of
>> them elsewhere (calling xmalloc from compat/, for example), so this is
>> just a rant, not an objection.
>
> There is intended behaviour difference between xmalloc() and
> malloc() that justifies your "layering violation" observation. A
> low level library replacement implemented in compat/ should not call
> die() when it fails to obtain memory and instead report an error.
>
> But it is unclear git_mkstemp_mode() and some others in wrapper.c
> fall into the same category. With the posted implementation above,
> the end result is how the platform that lack working mkstemp() would
> implement it.We'd have a problem if git_mkstemp_mode() depended on mkstemp(). It doesn't now, but if it is ever turned into a mkstemp() wrapper then it would fall flat on its face, but only on Windows.
> If we were to do something to make it "cleaner", I suspect that the > above implementatoin of mkstemp() can be moved out of compat/mingw.c > and made reusable by _anybody_ who lack mkstemp(), just like we ship > memmem() emulation for anybody who lack it in contrib/memmem.c
All other platforms seem to have a native mkstemp(). There may still be interest in using our own if the native implementation is inferior, e.g. uses a weak RNG and/or doesn't retry.
We could also convert the handful of mkstemp() callers (two if we ignore reftable/) to use git_mkstemp_mode(). One of them is quite tempting for the code deduplication aspect alone (see below)..
René
--- wrapper.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-)
diff --git a/wrapper.c b/wrapper.c index 1c3c970080..77ca923040 100644 --- a/wrapper.c +++ b/wrapper.c @@ -439,24 +439,7 @@ FILE *fopen_or_warn(const char *path, const char *mode) int xmkstemp(char *filename_template) { - int fd; - char origtemplate[PATH_MAX]; - strlcpy(origtemplate, filename_template, sizeof(origtemplate)); - - fd = mkstemp(filename_template); - if (fd < 0) { - int saved_errno = errno; - const char *nonrelative_template; - - if (strlen(filename_template) != strlen(origtemplate)) - filename_template = origtemplate; - - nonrelative_template = absolute_path(filename_template); - errno = saved_errno; - die_errno("Unable to create temporary file '%s'", - nonrelative_template); - } - return fd; + return xmkstemp_mode(filename_template, 0600); } /* Adapted from libiberty's mkstemp.c. */ -- 2.37.0