Re: [PATCH v3 2/3] Introduce GIT_MMAP_LIMIT to allow testing expected mmap size
On Aug 22, 2014, at 6:31 PM, Junio C Hamano wrote: > Steffen Prohaska writes: > + if (limit == -1) { + const char *env = getenv("GIT_MMAP_LIMIT"); + limit = env ? atoi(env) * 1024 : 0; >> >> ... this should then be changed to atol(env), and ... > > In the real codepath (not debugging aid like this) we try to avoid > atoi/atol so that we can catch errors like feeding "123Q" and > parsing it as 123. > > But it is OK to be loose in an debugging aid. If I were doing > this code, I actually would call git_parse_ulong() and not > define it in terms of kilobytes, though. Thanks for suggesting this. I wasn't aware of git_parse_ulong(). I think it makes very much sense to use it, even though it's only a testing aid. I'll send a PATCH v5 series. 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 v3 2/3] Introduce GIT_MMAP_LIMIT to allow testing expected mmap size
Junio C Hamano writes: > Steffen Prohaska writes: > + if (limit == -1) { + const char *env = getenv("GIT_MMAP_LIMIT"); + limit = env ? atoi(env) * 1024 : 0; >> >> ... this should then be changed to atol(env), and ... > > In the real codepath (not debugging aid like this) we try to avoid > atoi/atol so that we can catch errors like feeding "123Q" and > parsing it as 123. Sorry for hitting by mistake before finishing the paragraph, which should have concluded with: But it is OK to be loose in an debugging aid. If I were doing this code, I actually would call git_parse_ulong() and not define it in terms of kilobytes, though. -- 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 v3 2/3] Introduce GIT_MMAP_LIMIT to allow testing expected mmap size
Steffen Prohaska writes: >>> + if (limit == -1) { >>> + const char *env = getenv("GIT_MMAP_LIMIT"); >>> + limit = env ? atoi(env) * 1024 : 0; > > ... this should then be changed to atol(env), and ... In the real codepath (not debugging aid like this) we try to avoid atoi/atol so that we can catch errors like feeding "123Q" and parsing it as 123. -- 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 v3 2/3] Introduce GIT_MMAP_LIMIT to allow testing expected mmap size
On Aug 22, 2014, at 12:26 AM, Junio C Hamano wrote: > Steffen Prohaska writes: > >> Similar to testing expectations about malloc with GIT_ALLOC_LIMIT (see >> commit d41489), it can be useful to test expectations about mmap. >> >> This introduces a new environment variable GIT_MMAP_LIMIT to limit the >> largest allowed mmap length (in KB). xmmap() is modified to check the >> limit. Together with GIT_ALLOC_LIMIT tests can now easily confirm >> expectations about memory consumption. >> >> GIT_ALLOC_LIMIT will be used in the next commit to test that data will > > I smell the need for s/ALLOC/MMAP/ here, but perhaps you did mean > ALLOC (I won't know until I check 3/3 ;-) You are right. >> diff --git a/sha1_file.c b/sha1_file.c >> index 00c07f2..88d64c0 100644 >> --- a/sha1_file.c >> +++ b/sha1_file.c >> @@ -663,10 +663,25 @@ void release_pack_memory(size_t need) >> ; /* nothing */ >> } >> >> +static void mmap_limit_check(size_t length) >> +{ >> +static int limit = -1; > > Perhaps you want ssize_t here? I see mmap() as a tool to handle a > lot more data than a single malloc() typically would ;-) so previous > mistakes by other people would not be a good excuse. Maybe; and ... >> +if (limit == -1) { >> +const char *env = getenv("GIT_MMAP_LIMIT"); >> +limit = env ? atoi(env) * 1024 : 0; ... this should then be changed to atol(env), and ... >> +} >> +if (limit && length > limit) >> +die("attempting to mmap %"PRIuMAX" over limit %d", >> +(intmax_t)length, limit); ... here PRIuMAX and (uintmax_t); (uintmax_t) also for length. I'll fix it and also GIT_ALLOC_LIMIT. Steffen >> +} >> + >> void *xmmap(void *start, size_t length, >> int prot, int flags, int fd, off_t offset) >> { >> -void *ret = mmap(start, length, prot, flags, fd, offset); >> +void *ret; >> + >> +mmap_limit_check(length); >> +ret = mmap(start, length, prot, flags, fd, offset); >> if (ret == MAP_FAILED) { >> if (!length) >> return NULL; -- 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 v3 2/3] Introduce GIT_MMAP_LIMIT to allow testing expected mmap size
Steffen Prohaska writes: > Similar to testing expectations about malloc with GIT_ALLOC_LIMIT (see > commit d41489), it can be useful to test expectations about mmap. > > This introduces a new environment variable GIT_MMAP_LIMIT to limit the > largest allowed mmap length (in KB). xmmap() is modified to check the > limit. Together with GIT_ALLOC_LIMIT tests can now easily confirm > expectations about memory consumption. > > GIT_ALLOC_LIMIT will be used in the next commit to test that data will I smell the need for s/ALLOC/MMAP/ here, but perhaps you did mean ALLOC (I won't know until I check 3/3 ;-) > be streamed to an external filter without mmaping the entire file. > > [commit d41489]: d41489a6424308dc9a0409bc2f6845aa08bd4f7d Add more large > blob test cases > > Signed-off-by: Steffen Prohaska > --- > sha1_file.c | 17 - > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/sha1_file.c b/sha1_file.c > index 00c07f2..88d64c0 100644 > --- a/sha1_file.c > +++ b/sha1_file.c > @@ -663,10 +663,25 @@ void release_pack_memory(size_t need) > ; /* nothing */ > } > > +static void mmap_limit_check(size_t length) > +{ > + static int limit = -1; Perhaps you want ssize_t here? I see mmap() as a tool to handle a lot more data than a single malloc() typically would ;-) so previous mistakes by other people would not be a good excuse. > + if (limit == -1) { > + const char *env = getenv("GIT_MMAP_LIMIT"); > + limit = env ? atoi(env) * 1024 : 0; > + } > + if (limit && length > limit) > + die("attempting to mmap %"PRIuMAX" over limit %d", > + (intmax_t)length, limit); > +} > + > void *xmmap(void *start, size_t length, > int prot, int flags, int fd, off_t offset) > { > - void *ret = mmap(start, length, prot, flags, fd, offset); > + void *ret; > + > + mmap_limit_check(length); > + ret = mmap(start, length, prot, flags, fd, offset); > if (ret == MAP_FAILED) { > if (!length) > return NULL; -- 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