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

From: Alexander Potashev <aspotashev@gmail.com>
Date: Jun 4, 2009, 01:58 UTC
Message-ID: <2325a7950906031858oed495d0x406ea375ff08eb28@mail.gmail.com>
In-Reply-To: <2325a7950906031855t1977448lbb1c8aa671c72f3d@mail.gmail.com>

Hi Jeff, I was also thinking (about 1 month ago) about a helper function similar to your 'diesys', but I never thought that adding yet another "backend" function (diesys_routine) is reasonable.

Following your approach, you will need to add 'set_diesys_routine' and call it along with 'set_die_routine' (however, if you want to use 'diesys', but there are several places in 'daemon.c' and 'fast-import.c' where 'die' is being used to write 'strerror(errno)').

I think, we should just keep the backend interface as is and implement 'diesys' through 'die_routine'.

Alexander

2009/6/3 Jeff King <peff@peff.net>:

Show 122 quoted lines

> 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;
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>