Re: [PATCH] add strerror(errno) to die() calls where applicable

[Jeff King <peff@peff.net>]

On Tue, Jun 02, 2009 at 11:34:33PM +0200, Thomas Rast wrote:

> Lots of die() calls did not actually report the kind of error, which
> can leave the user confused as to the real problem.  Add a
> strerror(errno) where the die() is immediately preceded by a
> system/library call that sets errno on failure, or by one of the
> following that wrap such calls:

I like this, as I remember being frustrated in the past by "cannot $foo"
messages with no indication of the cause of the error. My only questions
or concerns with such a patch would be:

  1. How did you determine the set of callsites? Did you check that each
     non-syscall function always sets errno? Are there are functions
     which are setting errno which could also be included?

  2. Extra error conditions may leak information about the filesystem to
     people feeding bogus paths to upload-pack. I didn't see anything
     obvious in your patch that would cause this, but it is something to
     consider.

  3. This is such a common thing to do, I wonder if we would be better
     off adding a "diesys" function that appends ": strerror(errno)"
     to the emitted error. Something like the (totally untested) patch
     below.

And a few comments on the patch itself:

> @@ -109,7 +113,8 @@ int is_directory(const char *path)
>  	} else {
>  		const char *cwd = get_pwd_cwd();
>  		if (!cwd)
> -			die("Cannot determine the current working directory");
> +			die("Cannot determine the current working directory",
> +			    strerror(errno));

Missing ": %s" here?

> -		die("closing file %s: %s", path, strerror(errno));
> +		die("closing file '%s': %s", path, strerror(errno));

This one is actually just a style change, though I think it is
worthwhile (and there are a few others like it).


The diesys patch is below.

---
diff --git a/git-compat-util.h b/git-compat-util.h
index 4236647..410ac87 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -155,6 +155,7 @@
 /* General helper functions */
 extern void usage(const char *err) NORETURN;
 extern void die(const char *err, ...) NORETURN __attribute__((format (printf, 1, 2)));
+extern void diesys(const char *err, ...) NORETURN __attribute__((format (printf, 1, 2)));
 extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
diff --git a/usage.c b/usage.c
index 820d09f..da5e58e 100644
--- a/usage.c
+++ b/usage.c
@@ -12,6 +12,14 @@ static void report(const char *prefix, const char *err, va_list params)
 	fprintf(stderr, "%s%s\n", prefix, msg);
 }
 
+static void report_sys(int err, const char *prefix, const char *fmt, va_list
+		params)
+{
+	char msg[1024];
+	vsnprintf(msg, sizeof(msg), fmt, params);
+	fprintf(stderr, "%s%s: %s\n", prefix, msg, hstrerror(err));
+}
+
 static NORETURN void usage_builtin(const char *err)
 {
 	fprintf(stderr, "usage: %s\n", err);
@@ -24,6 +32,12 @@ static NORETURN void die_builtin(const char *err, va_list params)
 	exit(128);
 }
 
+static NORETURN void diesys_builtin(int err, const char *fmt, va_list params)
+{
+	report_sys(err, "fatal: ", fmt, params);
+	exit(128);
+}
+
 static void error_builtin(const char *err, va_list params)
 {
 	report("error: ", err, params);
@@ -38,6 +52,7 @@ static void warn_builtin(const char *warn, va_list params)
  * (ugh), so keep things static. */
 static void (*usage_routine)(const char *err) NORETURN = usage_builtin;
 static void (*die_routine)(const char *err, va_list params) NORETURN = die_builtin;
+static void (*diesys_routine)(int err, const char *fmt, va_list params) NORETURN = diesys_builtin;
 static void (*error_routine)(const char *err, va_list params) = error_builtin;
 static void (*warn_routine)(const char *err, va_list params) = warn_builtin;
 
@@ -60,6 +75,16 @@ void die(const char *err, ...)
 	va_end(params);
 }
 
+void diesys(const char *fmt, ...)
+{
+	va_list params;
+	int err = errno;
+
+	va_start(params, fmt);
+	diesys_routine(err, fmt, params);
+	va_end(params);
+}
+
 int error(const char *err, ...)
 {
 	va_list params;