Re: [RFC][PATCH] mingw: avoid mktemp() in mkstemp() implementation
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(-)
Show changes to wrapper.c +1 −19
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
René Scharfe
