From: Arnout Engelen <arnouten@bzzt.net>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] log which temporary file could not be created
Date: Thu, 4 Nov 2010 01:24:50 +0100 [thread overview]
Message-ID: <20101104002450.GQ9348@bzzt.net> (raw)
In-Reply-To: <20101021205800.GC12685@burratino>
Thanks for your feedback.
Modified patch all the way below ;)
On Thu, Oct 21, 2010 at 03:58:00PM -0500, Jonathan Nieder wrote:
> A strdup() takes much more than a strncpy() last time I checked. So I'm
> glad to see you're using the latter. :)
:)
> Is exit(-1) portable? I'd suggest using die() or usage().
I think it is, but 'usage' is indeed neater. Done.
> > + // temporary file in nonexistent directory
> // does not work on C89-based compilers (sadly, there are still many
> in wide use).
Changed to /* */
> > + xmkstemp(strdup("/tmp/does/not/exist/testXXXXXX"));
>
> Probably better to take a parameter with the filename.
Done
> > +++ b/t/t0007-mktemp.sh
> > + test-mktemp $2 &>actual
>
> &> does not work on most shells.
Changed to '2>', tested with zsh, ksh and bash.
> Might be clearer to spell these out:
>
> 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
> '
>
> This probably would belong in t0070-fundamental.sh.
Done and moved
> > --- a/wrapper.c
> > +++ b/wrapper.c
> > @@ -212,20 +212,42 @@ FILE *xfdopen(int fd, const char *mode)
> > int xmkstemp(char *template)
> > {
> > int fd;
> > + char originalTemplate[255];
>
> The existing convention is to use lowercase, brief names, with
> underscores separating words where appropraite. See "Chapter 4:
> Naming" from Documentation/CodingStyle in linux-2.6 for explanation.
Changed to 'origtemplate'
> > + strncpy(originalTemplate, template, 255);
> > + originalTemplate[254] = '\0';
>
> Maybe strlcpy() would be simpler?
Jep, thanks, didn't know that one. Changed.
> > fd = mkstemp(template);
> > - if (fd < 0)
> > - die_errno("Unable to create temporary file");
> > + if (fd < 0) {
> > + if (strlen(template) == 0) {
> > + template = originalTemplate;
> > + }
>
> Useless use of strlen(). You probably wanted 'if (!template[0])'.
I thought strlen() would be a bit neater/more readable - but changed.
> Unnecessary brace clutter (see Documentation/CodingGuidelines for
> explanation).
Changed
> > + if (*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));
>
> Okay. is_absolute_path() might be helpful on Windows.
Changed.
Verified the testcases still succeed.
The patch below:
- copies the original template to the stack, to be able to log it in case
mkstemp clears it
- on failure, logs the modified template, if any, or the original one
- logs the CWD when the template is relative
- adds a test testing this gives sane output (and doesn't crash, etc)
Signed-off-by: Arnout Engelen <arnouten@bzzt.net>
---
Makefile | 1 +
t/t0070-fundamental.sh | 13 +++++++++++++
test-mktemp.c | 14 ++++++++++++++
wrapper.c | 26 ++++++++++++++++++++++----
4 files changed, 50 insertions(+), 4 deletions(-)
create mode 100644 test-mktemp.c
diff --git a/Makefile b/Makefile
index 1f1ce04..30aafa2 100644
--- a/Makefile
+++ b/Makefile
@@ -428,6 +428,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..30e266a
--- /dev/null
+++ b/test-mktemp.c
@@ -0,0 +1,14 @@
+/*
+ * test-mktemp.c: code to exercise the creation of temporary files
+ */
+#include "wrapper.h"
+
+int main(int argc, char *argv[])
+{
+ if (argc != 2) {
+ usage("Expected 1 parameter defining the temporary file template");
+ }
+ xmkstemp(strdup(argv[1]));
+
+ return 0;
+}
diff --git a/wrapper.c b/wrapper.c
index fd8ead3..b5f10d1 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -212,20 +212,38 @@ FILE *xfdopen(int fd, const char *mode)
int xmkstemp(char *template)
{
int fd;
+ char origtemplate[255];
+ strlcpy(origtemplate, template, 255);
fd = mkstemp(template);
- if (fd < 0)
- die_errno("Unable to create temporary file");
+ if (fd < 0) {
+ 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));
+ }
return fd;
}
int xmkstemp_mode(char *template, int mode)
{
int fd;
+ char origtemplate[255];
+ strlcpy(origtemplate, template, 255);
fd = git_mkstemp_mode(template, mode);
- if (fd < 0)
- die_errno("Unable to create temporary file");
+ if (fd < 0) {
+ 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));
+ }
return fd;
}
--
1.7.2.3
prev parent reply other threads:[~2010-11-04 0:25 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-10-09 20:17 [PATCH] log which temporary file could not be created Arnout Engelen
2010-10-10 2:41 ` Jonathan Nieder
2010-10-10 10:33 ` Arnout Engelen
2010-10-10 18:09 ` Jonathan Nieder
2010-10-10 18:56 ` Arnout Engelen
2010-10-12 20:19 ` [PATCH] send-pack: avoid redundant "pack-objects died with strange error" Jonathan Nieder
2010-10-16 6:04 ` [PATCH v2] " Jonathan Nieder
2010-10-16 9:25 ` Johannes Sixt
2010-10-16 17:09 ` [PATCH v3] " Jonathan Nieder
2010-10-12 3:56 ` [PATCH] log which temporary file could not be created Junio C Hamano
2010-10-18 9:20 ` Arnout Engelen
[not found] ` <20101021205800.GC12685@burratino>
2010-11-04 0:24 ` Arnout Engelen [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20101104002450.GQ9348@bzzt.net \
--to=arnouten@bzzt.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).