Re: [PATCH v5 2/4] Change GIT_ALLOC_LIMIT check to use git_parse_ulong()
On Mon, Aug 25, 2014 at 05:06:22PM +0200, Steffen Prohaska wrote: > I think it's reasonable that GIT_ALLOC_LIMIT=0 means "no limit", so that > the limit can easily be disabled temporarily. IMHO, GIT_ALLOC_LIMIT= (i.e., the empty string) would be a good way to say that (and I guess that even works currently, due to the way atoi works, but I suspect git_parse_ulong might complain). It is probably not worth worrying about too much. This is not even a user-facing interface, and the test scripts just set it to 0. So I'd be OK going that direction, or just leaving it as-is. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/4] Change GIT_ALLOC_LIMIT check to use git_parse_ulong()
On Aug 25, 2014, at 1:38 PM, Jeff King wrote:
> On Sun, Aug 24, 2014 at 06:07:44PM +0200, Steffen Prohaska wrote:
>
>> 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).
I think it's reasonable that GIT_ALLOC_LIMIT=0 means "no limit", so that
the limit can easily be disabled temporarily.
But I could change the sentinel and handle 0 like:
if (git_parse_ulong(env, &val)) {
if (!val) {
val = SIZE_MAX;
}
}
Maybe we should do this.
>> +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);
I think you're right. I'll change it.
Steffen--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/4] Change GIT_ALLOC_LIMIT check to use git_parse_ulong()
On Sun, Aug 24, 2014 at 06:07:44PM +0200, Steffen Prohaska wrote:
> 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).
> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 2/4] Change GIT_ALLOC_LIMIT check to use git_parse_ulong()
GIT_ALLOC_LIMIT limits xmalloc()'s size, which is of type size_t.
Better use git_parse_ulong() to parse the environment variable, so that
the postfixes 'k', 'm', and 'g' can be used; and use size_t to store the
limit for consistency. The change to size_t has no direct practical
impact, because we use GIT_ALLOC_LIMIT to test small sizes.
The cast of size in the call to die() is changed to uintmax_t to match
the format string PRIuMAX.
Signed-off-by: Steffen Prohaska
---
t/t1050-large.sh | 2 +-
wrapper.c| 16 ++--
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/t/t1050-large.sh b/t/t1050-large.sh
index aea4936..e7657ab 100755
--- a/t/t1050-large.sh
+++ b/t/t1050-large.sh
@@ -13,7 +13,7 @@ test_expect_success setup '
echo X | dd of=large2 bs=1k seek=2000 &&
echo X | dd of=large3 bs=1k seek=2000 &&
echo Y | dd of=huge bs=1k seek=2500 &&
- GIT_ALLOC_LIMIT=1500 &&
+ GIT_ALLOC_LIMIT=1500k &&
export GIT_ALLOC_LIMIT
'
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) {
+ 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;
}
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);
}
try_to_free_t set_try_to_free_routine(try_to_free_t routine)
--
2.1.0.8.gd3b6067
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
