Re: [PATCH v5 2/4] Change GIT_ALLOC_LIMIT check to use git_parse_ulong()
- From
Jeff King <peff@peff.net>
- Date
- Aug 25, 2014, 11:38 UTC
- Message-ID
- <20140825113856.GA17288@peff.net>
- In-Reply-To
- <1408896466-23149-3-git-send-email-prohaska@zib.de>
On Sun, Aug 24, 2014 at 06:07:44PM +0200, Steffen Prohaska wrote:
Show 14 quoted lines
> diff --git a/wrapper.c b/wrapper.c
> index bc1bfb8..69d1c9b 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -11,14 +11,18 @@ static void (*try_to_free_routine)(size_t size) = do_nothing;
>
> static void memory_limit_check(size_t size)
> {
> - static int limit = -1;
> - if (limit == -1) {
> - const char *env = getenv("GIT_ALLOC_LIMIT");
> - limit = env ? atoi(env) * 1024 : 0;
> + static size_t limit = SIZE_MAX;
> + if (limit == SIZE_MAX) {You use SIZE_MAX as the sentinel for "not set", and 0 as the sentinel for "no limit". That seems kind of backwards.
I guess you are inheriting this from the existing code, which lets GIT_ALLOC_LIMIT=0 mean "no limit". I'm not sure if we want to keep that or not (it would be backwards incompatible to change it, but we are already breaking compatibility here by assuming bytes rather than kilobytes; I think that's OK because this is not a documented feature, or one intended to be used externally).
Show 7 quoted lines
> + const char *var = "GIT_ALLOC_LIMIT";
> + unsigned long val = 0;
> + const char *env = getenv(var);
> + if (env && !git_parse_ulong(env, &val))
> + die("Failed to parse %s", var);
> + limit = val;
> }This and the next patch both look OK to me, but I notice this part is largely duplicated between the two. We already have git_env_bool to do a similar thing for boolean environment variables. Should we do something similar like:
diff --git a/config.c b/config.c index 058505c..11919eb 100644 --- a/config.c +++ b/config.c @@ -1122,6 +1122,14 @@ int git_env_bool(const char *k, int def) return v ? git_config_bool(k, v) : def; } +unsigned long git_env_ulong(const char *k, unsigned long val) +{ + const char *v = getenv(k); + if (v && !git_parse_ulong(k, &val)) + die("failed to parse %s", k); + return val; +} + int git_config_system(void) { return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0); It's not a lot of code, but I think the callers end up being much easier to read: if (limit == SIZE_MAX) limit = git_env_ulong("GIT_ALLOC_LIMIT", 0); > if (limit && size > limit) > - die("attempting to allocate %"PRIuMAX" over limit %d", > - (intmax_t)size, limit); > + die("attempting to allocate %"PRIuMAX" over limit %"PRIuMAX, > + (uintmax_t)size, (uintmax_t)limit); This part is duplicated, too, though I do not know if the infrastructure to avoid that is worth the trouble. Unless you wanted to do a whole: check_limit(&limit, "GIT_ALLOC_LIMIT", size); or something, but I am also not convinced that is not just obfuscating things. -Peff