login
Search
This patch has been submitted/discussed before, but that version doesn't apply cleanly to the newest git anymore, so here's an updated version. It improves diagnostic error messages when creating a temporary file fails. Signed-off-by: Arnout Engelen <arnouten@bzzt.net> --- Makefile | 1 + t/t0070-fundamental.sh | 13 +++++++++++++ test-mktemp.c | 16 ++++++++++++++++ wrapper.c | 29 +++++++++++++++++++++++++---- wrapper.h | 4 ++++ 5 files changed, 59 insertions(+), 4 deletions(-) create mode 100644 test-mktemp.c create mode 100644 wrapper.h diff --git a/Makefile b/Makefile index 7a5fb69..10cfab2 100644 --- a/Makefile +++ b/Makefile @@ -434,6 +434,7 @@ TEST_PROGRAMS_NEED_X += test-string-pool TEST_PROGRAMS_NEED_X += test-svn-fe TEST_PROGRAMS_NEED_X += test-treap TEST_PROGRAMS_NEED_X += test-index-version +TEST_PROGRAMS_NEED_X += test-mktemp TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X)) diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 680d7d6..9bee8bf 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -12,4 +12,17 @@ test_expect_success 'character classes (isspace, isalpha etc.)' ' test-ctype ' +test_expect_success 'mktemp to nonexistent directory prints filename' ' + test_must_fail test-mktemp doesnotexist/testXXXXXX 2>err && + grep "doesnotexist/test" err +' + +test_expect_success POSIXPERM 'mktemp to unwritable directory prints filename' ' + mkdir cannotwrite && + chmod -w cannotwrite && + test_when_finished "chmod +w cannotwrite" && + test_must_fail test-mktemp cannotwrite/testXXXXXX 2>err && + grep "cannotwrite/test" err +' + test_done diff --git a/test-mktemp.c b/test-mktemp.c new file mode 100644 index 0000000..d392fa7 --- /dev/null +++ b/test-mktemp.c @@ -0,0 +1,16 @@ +/* + * test-mktemp.c: code to exercise the creation of temporary files + */ +#include <string.h> +#include "git-compat-util.h" +#include ...
This is a Linux libc/glibc-specific extension, alas. On other platforms
it would print "(null)" or segfault.
Here's some other assorted tweaks. I didn't bother to find your old
patch in the mailing list archive to take a fuller change description
from.
Hope that helps,
Jonathan
---
diff --git a/test-mktemp.c b/test-mktemp.c
index d392fa7..2e3b134 100644
--- a/test-mktemp.c
+++ b/test-mktemp.c
@@ -7,10 +7,9 @@
int main(int argc, char *argv[])
{
- if (argc != 2) {
+ if (argc != 2)
usage("Expected 1 parameter defining the temporary file template");
- }
- xmkstemp(strdup(argv[1]));
+ xmkstemp(xstrdup(argv[1]));
return 0;
}
diff --git a/wrapper.c b/wrapper.c
index 6640c87..cb9c9ad 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -197,19 +197,18 @@ FILE *xfdopen(int fd, const char *mode)
int xmkstemp(char *template)
{
int fd;
- char origtemplate[255];
- strlcpy(origtemplate, template, 255);
+ char origtemplate[256];
+ strlcpy(origtemplate, template, sizeof(origtemplate));
fd = mkstemp(template);
if (fd < 0) {
+ int saved_errno = errno;
+
if (!template[0])
template = origtemplate;
-
- if (is_absolute_path(template))
- die_errno("Unable to create temporary file '%s'", template);
- else
- die_errno("Unable to create temporary file '%s' at %s",
- template, getcwd(NULL, 0));
+ template = make_nonrelative_path(template);
+ errno = saved_errno;
+ die_errno("Unable to create temporary file '%s'", template);
}
return fd;
}
@@ -330,19 +329,18 @@ int gitmkstemps(char *pattern, int suffix_len)
int xmkstemp_mode(char *template, int mode)
{
int fd;
- char origtemplate[255];
- strlcpy(origtemplate, template, 255);
+ char origtemplate[256];
+ strlcpy(origtemplate, template, sizeof(origtemplate));
fd = git_mkstemp_mode(template, mode);
if (fd < 0) {
+ int saved_errno = errno;
+
if (!template[0])
template = origtemplate;
-
- if (is_absolute_path(template))
- die_errno("Unable to create ...Why "255"? It may happen to be sufficiently large for the current callers, but what provisions if any are made to help the compiler or the runtime protect us from new and broken callers? Use of strlcpy() there hides the issue from the runtime by avoiding segfault, but it actively harms us by making the Somewhat questionable... Why doesn't this say "extern"? What happend to another copy of this line in git-compat-util.h? IOW, what protects this and the other one from drifting apart? Why doesn't users include wrapper.h but only wrapper.c? Why yet another header is needed only for this function and nothing else? Why isn't this protected with the standard #ifndef .../#define .../#endif? --
Thanks to you and Jonathan again for the feedback. Only in a small way: when a bigger template is encountered and the mkstemp call succeeds, there is no problem. Only when xmkstemp fails *and* clears the template, the diagnostic error message shows a truncated version of the original. I *could* dynamically allocate space for the original template string, but that would mean I'd need to do a malloc() instead of putting the buffer on the stack like this, and free() it afterwards. I'm not too concerned about the performance hit here (presumably the I/O that comes with creating and using the temporary file here is orders of magnitude slower than that malloc() anyway), but it would also make the code a bit less easy to read. What do you think would be preferable here, a simple fixed-length buffer on the stack that might cause a truncated error message or a dynamically-allocated Agreed, this whole file is unneeded and, well, wrong anyway. I'll remove wrapper.h and apply Jonathan's improvements some time this week, unless of course someone beats me to it :). Kind regards, Arnout --
Why not use PATH_MAX instead of 255? P.S. I'm sory for cutting up CC list... -- Jakub Narebski Warsaw, Poland ShadeHawk on #git --
The advantage I can see to 256 is a small speed-up in the "no errors" code path. Since the I/O would tend to be much more costly and PATH_MAX is self explanatory, using PATH_MAX does seem cleaner. I know you all are aware of this already; just thought I'd mention it while forwarding the original message to Arnout. :) Jonathan --
Ah, ok, it seems that I misread the patch. This copy you are making is not used to actually construct the filename used for creating the temporary file, so there is no risk the function misbehaving; we would just give a truncated error report, which is no worse than what we have been giving the users anyway. --
Updated version of the patch, taking into account the feedback from this thread. (the use of a fixed-length buffer to temporarily store the template for the error path is now sort of moot since make_nonrelative_path doesn't allow filenames larger than PATH_MAX anyway) Signed-off-by: Arnout Engelen <arnouten@bzzt.net> --- Makefile | 1 + t/t0070-fundamental.sh | 13 +++++++++++++ test-mktemp.c | 15 +++++++++++++++ wrapper.c | 32 ++++++++++++++++++++++++++++---- 4 files changed, 57 insertions(+), 4 deletions(-) create mode 100644 test-mktemp.c diff --git a/Makefile b/Makefile index 57d9c65..03a51cb 100644 --- a/Makefile +++ b/Makefile @@ -434,6 +434,7 @@ TEST_PROGRAMS_NEED_X += test-string-pool TEST_PROGRAMS_NEED_X += test-svn-fe TEST_PROGRAMS_NEED_X += test-treap TEST_PROGRAMS_NEED_X += test-index-version +TEST_PROGRAMS_NEED_X += test-mktemp TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X)) diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 680d7d6..9bee8bf 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -12,4 +12,17 @@ test_expect_success 'character classes (isspace, isalpha etc.)' ' test-ctype ' +test_expect_success 'mktemp to nonexistent directory prints filename' ' + test_must_fail test-mktemp doesnotexist/testXXXXXX 2>err && + grep "doesnotexist/test" err +' + +test_expect_success POSIXPERM 'mktemp to unwritable directory prints filename' ' + mkdir cannotwrite && + chmod -w cannotwrite && + test_when_finished "chmod +w cannotwrite" && + test_must_fail test-mktemp cannotwrite/testXXXXXX 2>err && + grep "cannotwrite/test" err +' + test_done diff --git a/test-mktemp.c b/test-mktemp.c new file mode 100644 index 0000000..00fdd78 --- /dev/null +++ b/test-mktemp.c @@ -0,0 +1,15 @@ +/* + * test-mktemp.c: code to exercise the creation of temporary files + */ +#include <string.h> +#include "git-compat-util.h" + +int main(int argc, char ...
Hi again, [...] This text above the "---" becomes part of the log message when a patch is committed to git.git, so it is best to make it self-contained. The usual advice is "describe the current behavior, why the proposed git-compat-util.h takes care of portably including system headers in the right order. (For example, #include-ing <string.h> before setting _POSIX_SOURCE will cause some symbols not to be defined in _other_ headers on some operating systems, iirc.) I'd suggest removing the redundant #include <string.h>. It would be more usual not to include a space between the '*' and 'nonrelative_template'. Thanks for your perseverance. --
Thanks, Jonathan. The important part of self-containedness is that people who are reading the resulting commit and the history that contains it should not have to unnecessarily refer to outside resources, especially the previous rounds that weren't satisfactory. The comparison with previous rounds however is a very valuable resource for reviewers on the mailing list, so don't remove what you wrote there, but move it below "---" lines. --
Thanks, Jonathan. The important part of self-containedness is that people who are reading the resulting commit and the history that contains it should not have to unnecessarily refer to outside resources, especially the previous rounds that weren't satisfactory. The comparison with previous rounds however is a very valuable resource for reviewers on the mailing list, so don't remove what you wrote there, but move it below "---" lines. --
Improve error messages when creating a temporary file fails. Before, when creating a temporary file failed, a generic 'Unable to create temporary file' message was printed. In some cases this could lead to confusion as to which directory should be checked for correct permissions etc. This patch adds the template for the temporary filename to the error message, converting it to an absolute path if needed. A test verifies that the template is indeed printed when pointing to a nonexistent or unwritable directory. (a copy of the original template is made in case mkstemp clears the template) Signed-off-by: Arnout Engelen <arnouten@bzzt.net> --- The use of a fixed-length buffer to temporarily store the template for the error path is now sort of moot since make_nonrelative_path doesn't allow filenames larger than PATH_MAX anyway. Some more tweaks based on Jonathan's feedback: xstrdup, #include of git-compat-util.h in the unittest and for whitespace around the '*' in pointer declarations. Makefile | 1 + t/t0070-fundamental.sh | 13 +++++++++++++ test-mktemp.c | 14 ++++++++++++++ wrapper.c | 32 ++++++++++++++++++++++++++++---- 4 files changed, 56 insertions(+), 4 deletions(-) create mode 100644 test-mktemp.c diff --git a/Makefile b/Makefile index 57d9c65..03a51cb 100644 --- a/Makefile +++ b/Makefile @@ -434,6 +434,7 @@ TEST_PROGRAMS_NEED_X += test-string-pool TEST_PROGRAMS_NEED_X += test-svn-fe TEST_PROGRAMS_NEED_X += test-treap TEST_PROGRAMS_NEED_X += test-index-version +TEST_PROGRAMS_NEED_X += test-mktemp TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X)) diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh index 680d7d6..9bee8bf 100755 --- a/t/t0070-fundamental.sh +++ b/t/t0070-fundamental.sh @@ -12,4 +12,17 @@ test_expect_success 'character classes (isspace, isalpha etc.)' ' test-ctype ' +test_expect_success 'mktemp to nonexistent directory prints filename' ' + test_must_fail ...
| Greg KH | Og dreams of kernels |
| Jens Axboe | [PATCH 31/33] Fusion: sg chaining support |
| Arnd Bergmann | Re: finding your own dead "CONFIG_" variables |
| Mark Brown |