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