{"thread":{"id":"37392","subject":"[PATCH v3 0/3] Stream fd to clean filter, GIT_MMAP_LIMIT","startedAt":"2014-08-21T16:05:07Z","lastAt":"2014-08-24T16:03:24Z","messageCount":9,"participants":["Steffen Prohaska","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":3},"messages":[{"id":"248064","messageId":"1408637110-15669-1-git-send-email-prohaska@zib.de","threadId":"37392","inReplyTo":null,"subject":"[PATCH v3 0/3] Stream fd to clean filter, GIT_MMAP_LIMIT","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-21T16:05:07Z","receivedAt":"2014-08-21T16:05:07Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"I revised the testing approach as discussed.  Patch 2/3 adds GIT_MMAP_LIMIT,\nwhich allows testing of memory expectations together with GIT_ALLOC_LIMIT.\n\nThe rest is unchanged compared to v2.\n\nSteffen Prohaska (3):\n  convert: Refactor would_convert_to_git() to single arg 'path'\n  Introduce GIT_MMAP_LIMIT to allow testing expected mmap size\n  convert: Stream from fd to required clean filter instead of mmap\n\n convert.c             | 60 +++++++++++++++++++++++++++++++++++++++++++++------\n convert.h             | 10 ++++++---\n sha1_file.c           | 46 ++++++++++++++++++++++++++++++++++++---\n t/t0021-conversion.sh | 24 ++++++++++++++++-----\n 4 files changed, 123 insertions(+), 17 deletions(-)\n\n-- \n2.1.0.6.gb452461\n"},{"id":"248067","messageId":"1408637110-15669-2-git-send-email-prohaska@zib.de","threadId":"37392","inReplyTo":"1408637110-15669-1-git-send-email-prohaska@zib.de","subject":"[PATCH v3 1/3] convert: Refactor would_convert_to_git() to single arg 'path'","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-21T16:05:08Z","receivedAt":"2014-08-21T16:05:08Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"It is only the path that matters in the decision whether to filter or\nnot.  Clarify this by making path the single argument of\nwould_convert_to_git().\n\nSigned-off-by: Steffen Prohaska <prohaska@zib.de>\n---\n convert.h   | 5 ++---\n sha1_file.c | 2 +-\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/convert.h b/convert.h\nindex 0c2143c..c638b33 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -40,10 +40,9 @@ extern int convert_to_working_tree(const char *path, const char *src,\n \t\t\t\t   size_t len, struct strbuf *dst);\n extern int renormalize_buffer(const char *path, const char *src, size_t len,\n \t\t\t      struct strbuf *dst);\n-static inline int would_convert_to_git(const char *path, const char *src,\n-\t\t\t\t       size_t len, enum safe_crlf checksafe)\n+static inline int would_convert_to_git(const char *path)\n {\n-\treturn convert_to_git(path, src, len, NULL, checksafe);\n+\treturn convert_to_git(path, NULL, 0, NULL, 0);\n }\n \n /*****************************************************************\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3f70b1d..00c07f2 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3144,7 +3144,7 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st,\n \tif (!S_ISREG(st->st_mode))\n \t\tret = index_pipe(sha1, fd, type, path, flags);\n \telse if (size <= big_file_threshold || type != OBJ_BLOB ||\n-\t\t (path && would_convert_to_git(path, NULL, 0, 0)))\n+\t\t (path && would_convert_to_git(path)))\n \t\tret = index_core(sha1, fd, size, type, path, flags);\n \telse\n \t\tret = index_stream(sha1, fd, size, type, path, flags);\n-- \n2.1.0.6.gb452461\n"},{"id":"248066","messageId":"1408637110-15669-3-git-send-email-prohaska@zib.de","threadId":"37392","inReplyTo":"1408637110-15669-1-git-send-email-prohaska@zib.de","subject":"[PATCH v3 2/3] Introduce GIT_MMAP_LIMIT to allow testing expected mmap size","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-21T16:05:09Z","receivedAt":"2014-08-21T16:05:09Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"Similar to testing expectations about malloc with GIT_ALLOC_LIMIT (see\ncommit d41489), it can be useful to test expectations about mmap.\n\nThis introduces a new environment variable GIT_MMAP_LIMIT to limit the\nlargest allowed mmap length (in KB).  xmmap() is modified to check the\nlimit.  Together with GIT_ALLOC_LIMIT tests can now easily confirm\nexpectations about memory consumption.\n\nGIT_ALLOC_LIMIT will be used in the next commit to test that data will\nbe streamed to an external filter without mmaping the entire file.\n\n[commit d41489]: d41489a6424308dc9a0409bc2f6845aa08bd4f7d Add more large\n    blob test cases\n\nSigned-off-by: Steffen Prohaska <prohaska@zib.de>\n---\n sha1_file.c | 17 ++++++++++++++++-\n 1 file changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 00c07f2..88d64c0 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -663,10 +663,25 @@ void release_pack_memory(size_t need)\n \t\t; /* nothing */\n }\n \n+static void mmap_limit_check(size_t length)\n+{\n+\tstatic int limit = -1;\n+\tif (limit == -1) {\n+\t\tconst char *env = getenv(\"GIT_MMAP_LIMIT\");\n+\t\tlimit = env ? atoi(env) * 1024 : 0;\n+\t}\n+\tif (limit && length > limit)\n+\t\tdie(\"attempting to mmap %\"PRIuMAX\" over limit %d\",\n+\t\t    (intmax_t)length, limit);\n+}\n+\n void *xmmap(void *start, size_t length,\n \tint prot, int flags, int fd, off_t offset)\n {\n-\tvoid *ret = mmap(start, length, prot, flags, fd, offset);\n+\tvoid *ret;\n+\n+\tmmap_limit_check(length);\n+\tret = mmap(start, length, prot, flags, fd, offset);\n \tif (ret == MAP_FAILED) {\n \t\tif (!length)\n \t\t\treturn NULL;\n-- \n2.1.0.6.gb452461\n"},{"id":"248065","messageId":"1408637110-15669-4-git-send-email-prohaska@zib.de","threadId":"37392","inReplyTo":"1408637110-15669-1-git-send-email-prohaska@zib.de","subject":"[PATCH v3 3/3] convert: Stream from fd to required clean filter instead of mmap","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-21T16:05:10Z","receivedAt":"2014-08-21T16:05:10Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"The data is streamed to the filter process anyway.  Better avoid mapping\nthe file if possible.  This is especially useful if a clean filter\nreduces the size, for example if it computes a sha1 for binary data,\nlike git media.  The file size that the previous implementation could\nhandle was limited by the available address space; large files for\nexample could not be handled with (32-bit) msysgit.  The new\nimplementation can filter files of any size as long as the filter output\nis small enough.\n\nThe new code path is only taken if the filter is required.  The filter\nconsumes data directly from the fd.  The original data is not available\nto git, so it must fail if the filter fails.\n\nThe environment variable GIT_MMAP_LIMIT, which has been introduced in\nthe previous commit is used to test that the expected code path is\ntaken.  A related test that exercises required filters is modified to\nverify that the data actually has been modified on its way from the file\nsystem to the object store.\n\nSigned-off-by: Steffen Prohaska <prohaska@zib.de>\n---\n convert.c             | 60 +++++++++++++++++++++++++++++++++++++++++++++------\n convert.h             |  5 +++++\n sha1_file.c           | 27 ++++++++++++++++++++++-\n t/t0021-conversion.sh | 24 ++++++++++++++++-----\n 4 files changed, 104 insertions(+), 12 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex cb5fbb4..463f6de 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -312,11 +312,12 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,\n struct filter_params {\n \tconst char *src;\n \tunsigned long size;\n+\tint fd;\n \tconst char *cmd;\n \tconst char *path;\n };\n \n-static int filter_buffer(int in, int out, void *data)\n+static int filter_buffer_or_fd(int in, int out, void *data)\n {\n \t/*\n \t * Spawn cmd and feed the buffer contents through its stdin.\n@@ -325,6 +326,7 @@ static int filter_buffer(int in, int out, void *data)\n \tstruct filter_params *params = (struct filter_params *)data;\n \tint write_err, status;\n \tconst char *argv[] = { NULL, NULL };\n+\tint fd;\n \n \t/* apply % substitution to cmd */\n \tstruct strbuf cmd = STRBUF_INIT;\n@@ -355,7 +357,17 @@ static int filter_buffer(int in, int out, void *data)\n \n \tsigchain_push(SIGPIPE, SIG_IGN);\n \n-\twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n+\tif (params->src) {\n+\t    write_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n+\t} else {\n+\t    /* dup(), because copy_fd() closes the input fd. */\n+\t    fd = dup(params->fd);\n+\t    if (fd < 0)\n+\t\twrite_err = error(\"failed to dup file descriptor.\");\n+\t    else\n+\t\twrite_err = copy_fd(fd, child_process.in);\n+\t}\n+\n \tif (close(child_process.in))\n \t\twrite_err = 1;\n \tif (write_err)\n@@ -371,7 +383,7 @@ static int filter_buffer(int in, int out, void *data)\n \treturn (write_err || status);\n }\n \n-static int apply_filter(const char *path, const char *src, size_t len,\n+static int apply_filter(const char *path, const char *src, size_t len, int fd,\n                         struct strbuf *dst, const char *cmd)\n {\n \t/*\n@@ -392,11 +404,12 @@ static int apply_filter(const char *path, const char *src, size_t len,\n \t\treturn 1;\n \n \tmemset(&async, 0, sizeof(async));\n-\tasync.proc = filter_buffer;\n+\tasync.proc = filter_buffer_or_fd;\n \tasync.data = &params;\n \tasync.out = -1;\n \tparams.src = src;\n \tparams.size = len;\n+\tparams.fd = fd;\n \tparams.cmd = cmd;\n \tparams.path = path;\n \n@@ -747,6 +760,24 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)\n \t}\n }\n \n+int would_convert_to_git_filter_fd(const char *path)\n+{\n+\tstruct conv_attrs ca;\n+\n+\tconvert_attrs(&ca, path);\n+\tif (!ca.drv)\n+\t    return 0;\n+\n+\t/* Apply a filter to an fd only if the filter is required to succeed.\n+\t * We must die if the filter fails, because the original data before\n+\t * filtering is not available.\n+\t */\n+\tif (!ca.drv->required)\n+\t    return 0;\n+\n+\treturn apply_filter(path, NULL, 0, -1, NULL, ca.drv->clean);\n+}\n+\n int convert_to_git(const char *path, const char *src, size_t len,\n                    struct strbuf *dst, enum safe_crlf checksafe)\n {\n@@ -761,7 +792,7 @@ int convert_to_git(const char *path, const char *src, size_t len,\n \t\trequired = ca.drv->required;\n \t}\n \n-\tret |= apply_filter(path, src, len, dst, filter);\n+\tret |= apply_filter(path, src, len, -1, dst, filter);\n \tif (!ret && required)\n \t\tdie(\"%s: clean filter '%s' failed\", path, ca.drv->name);\n \n@@ -778,6 +809,23 @@ int convert_to_git(const char *path, const char *src, size_t len,\n \treturn ret | ident_to_git(path, src, len, dst, ca.ident);\n }\n \n+void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,\n+\t\t\t      enum safe_crlf checksafe)\n+{\n+\tstruct conv_attrs ca;\n+\tconvert_attrs(&ca, path);\n+\n+\tassert(ca.drv);\n+\tassert(ca.drv->clean);\n+\n+\tif (!apply_filter(path, NULL, 0, fd, dst, ca.drv->clean))\n+\t\tdie(\"%s: clean filter '%s' failed\", path, ca.drv->name);\n+\n+\tca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);\n+\tcrlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);\n+\tident_to_git(path, dst->buf, dst->len, dst, ca.ident);\n+}\n+\n static int convert_to_working_tree_internal(const char *path, const char *src,\n \t\t\t\t\t    size_t len, struct strbuf *dst,\n \t\t\t\t\t    int normalizing)\n@@ -811,7 +859,7 @@ static int convert_to_working_tree_internal(const char *path, const char *src,\n \t\t}\n \t}\n \n-\tret_filter = apply_filter(path, src, len, dst, filter);\n+\tret_filter = apply_filter(path, src, len, -1, dst, filter);\n \tif (!ret_filter && required)\n \t\tdie(\"%s: smudge filter %s failed\", path, ca.drv->name);\n \ndiff --git a/convert.h b/convert.h\nindex c638b33..d9d853c 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -44,6 +44,11 @@ static inline int would_convert_to_git(const char *path)\n {\n \treturn convert_to_git(path, NULL, 0, NULL, 0);\n }\n+/* Precondition: would_convert_to_git_filter_fd(path) == true */\n+extern void convert_to_git_filter_fd(const char *path, int fd,\n+\t\t\t\t     struct strbuf *dst,\n+\t\t\t\t     enum safe_crlf checksafe);\n+extern int would_convert_to_git_filter_fd(const char *path);\n \n /*****************************************************************\n  *\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 88d64c0..1fe2d5f 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3089,6 +3089,29 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,\n \treturn ret;\n }\n \n+static int index_stream_convert_blob(unsigned char *sha1, int fd,\n+\t\t\t\t     const char *path, unsigned flags)\n+{\n+\tint ret;\n+\tconst int write_object = flags & HASH_WRITE_OBJECT;\n+\tstruct strbuf sbuf = STRBUF_INIT;\n+\n+\tassert(path);\n+\tassert(would_convert_to_git_filter_fd(path));\n+\n+\tconvert_to_git_filter_fd(path, fd, &sbuf,\n+\t\t\t\t write_object ? safe_crlf : SAFE_CRLF_FALSE);\n+\n+\tif (write_object)\n+\t\tret = write_sha1_file(sbuf.buf, sbuf.len, typename(OBJ_BLOB),\n+\t\t\t\t      sha1);\n+\telse\n+\t\tret = hash_sha1_file(sbuf.buf, sbuf.len, typename(OBJ_BLOB),\n+\t\t\t\t     sha1);\n+\tstrbuf_release(&sbuf);\n+\treturn ret;\n+}\n+\n static int index_pipe(unsigned char *sha1, int fd, enum object_type type,\n \t\t      const char *path, unsigned flags)\n {\n@@ -3156,7 +3179,9 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st,\n \tint ret;\n \tsize_t size = xsize_t(st->st_size);\n \n-\tif (!S_ISREG(st->st_mode))\n+\tif (type == OBJ_BLOB && path && would_convert_to_git_filter_fd(path))\n+\t\tret = index_stream_convert_blob(sha1, fd, path, flags);\n+\telse if (!S_ISREG(st->st_mode))\n \t\tret = index_pipe(sha1, fd, type, path, flags);\n \telse if (size <= big_file_threshold || type != OBJ_BLOB ||\n \t\t (path && would_convert_to_git(path)))\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex f890c54..d566508 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -153,17 +153,23 @@ test_expect_success 'filter shell-escaped filenames' '\n \t:\n '\n \n-test_expect_success 'required filter success' '\n-\tgit config filter.required.smudge cat &&\n-\tgit config filter.required.clean cat &&\n+test_expect_success 'required filter should filter data' '\n+\tgit config filter.required.smudge ./rot13.sh &&\n+\tgit config filter.required.clean ./rot13.sh &&\n \tgit config filter.required.required true &&\n \n \techo \"*.r filter=required\" >.gitattributes &&\n \n-\techo test >test.r &&\n+\tcat test.o >test.r &&\n \tgit add test.r &&\n+\n \trm -f test.r &&\n-\tgit checkout -- test.r\n+\tgit checkout -- test.r &&\n+\tcmp test.o test.r &&\n+\n+\t./rot13.sh <test.o >expected &&\n+\tgit cat-file blob :test.r >actual &&\n+\tcmp expected actual\n '\n \n test_expect_success 'required filter smudge failure' '\n@@ -189,6 +195,14 @@ test_expect_success 'required filter clean failure' '\n \techo test >test.fc &&\n \ttest_must_fail git add test.fc\n '\n+test_expect_success \\\n+'filtering large input to small output should use little memory' '\n+\tgit config filter.devnull.clean \"cat >/dev/null\" &&\n+\tgit config filter.devnull.required true &&\n+\tfor i in $(test_seq 1 30); do printf \"%1048576d\" 1; done >30MB &&\n+\techo \"30MB filter=devnull\" >.gitattributes &&\n+\tGIT_MMAP_LIMIT=1000 GIT_ALLOC_LIMIT=1000 git add 30MB\n+'\n \n test_expect_success EXPENSIVE 'filter large file' '\n \tgit config filter.largefile.smudge cat &&\n-- \n2.1.0.6.gb452461\n"},{"id":"248086","messageId":"xmqq1ts9qy24.fsf@gitster.dls.corp.google.com","threadId":"37392","inReplyTo":"1408637110-15669-3-git-send-email-prohaska@zib.de","subject":"Re: [PATCH v3 2/3] Introduce GIT_MMAP_LIMIT to allow testing expected mmap size","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-21T22:26:27Z","receivedAt":"2014-08-21T22:26:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska <prohaska@zib.de> writes:\n\n> Similar to testing expectations about malloc with GIT_ALLOC_LIMIT (see\n> commit d41489), it can be useful to test expectations about mmap.\n>\n> This introduces a new environment variable GIT_MMAP_LIMIT to limit the\n> largest allowed mmap length (in KB).  xmmap() is modified to check the\n> limit.  Together with GIT_ALLOC_LIMIT tests can now easily confirm\n> expectations about memory consumption.\n>\n> GIT_ALLOC_LIMIT will be used in the next commit to test that data will\n\nI smell the need for s/ALLOC/MMAP/ here, but perhaps you did mean\nALLOC (I won't know until I check 3/3 ;-)\n\n> be streamed to an external filter without mmaping the entire file.\n>\n> [commit d41489]: d41489a6424308dc9a0409bc2f6845aa08bd4f7d Add more large\n>     blob test cases\n>\n> Signed-off-by: Steffen Prohaska <prohaska@zib.de>\n> ---\n>  sha1_file.c | 17 ++++++++++++++++-\n>  1 file changed, 16 insertions(+), 1 deletion(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 00c07f2..88d64c0 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -663,10 +663,25 @@ void release_pack_memory(size_t need)\n>  \t\t; /* nothing */\n>  }\n>  \n> +static void mmap_limit_check(size_t length)\n> +{\n> +\tstatic int limit = -1;\n\nPerhaps you want ssize_t here?  I see mmap() as a tool to handle a\nlot more data than a single malloc() typically would ;-) so previous\nmistakes by other people would not be a good excuse.\n\n> +\tif (limit == -1) {\n> +\t\tconst char *env = getenv(\"GIT_MMAP_LIMIT\");\n> +\t\tlimit = env ? atoi(env) * 1024 : 0;\n> +\t}\n> +\tif (limit && length > limit)\n> +\t\tdie(\"attempting to mmap %\"PRIuMAX\" over limit %d\",\n> +\t\t    (intmax_t)length, limit);\n> +}\n> +\n>  void *xmmap(void *start, size_t length,\n>  \tint prot, int flags, int fd, off_t offset)\n>  {\n> -\tvoid *ret = mmap(start, length, prot, flags, fd, offset);\n> +\tvoid *ret;\n> +\n> +\tmmap_limit_check(length);\n> +\tret = mmap(start, length, prot, flags, fd, offset);\n>  \tif (ret == MAP_FAILED) {\n>  \t\tif (!length)\n>  \t\t\treturn NULL;\n"},{"id":"248112","messageId":"0342479D-7C9F-42E6-9B79-745AEDE57EB5@zib.de","threadId":"37392","inReplyTo":"xmqq1ts9qy24.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/3] Introduce GIT_MMAP_LIMIT to allow testing expected mmap size","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-22T13:46:11Z","receivedAt":"2014-08-22T13:46:11Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Aug 22, 2014, at 12:26 AM, Junio C Hamano <gitster@pobox.com> wrote:\n\n> Steffen Prohaska <prohaska@zib.de> writes:\n> \n>> Similar to testing expectations about malloc with GIT_ALLOC_LIMIT (see\n>> commit d41489), it can be useful to test expectations about mmap.\n>> \n>> This introduces a new environment variable GIT_MMAP_LIMIT to limit the\n>> largest allowed mmap length (in KB).  xmmap() is modified to check the\n>> limit.  Together with GIT_ALLOC_LIMIT tests can now easily confirm\n>> expectations about memory consumption.\n>> \n>> GIT_ALLOC_LIMIT will be used in the next commit to test that data will\n> \n> I smell the need for s/ALLOC/MMAP/ here, but perhaps you did mean\n> ALLOC (I won't know until I check 3/3 ;-)\n\nYou are right.\n\n\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 00c07f2..88d64c0 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -663,10 +663,25 @@ void release_pack_memory(size_t need)\n>> \t\t; /* nothing */\n>> }\n>> \n>> +static void mmap_limit_check(size_t length)\n>> +{\n>> +\tstatic int limit = -1;\n> \n> Perhaps you want ssize_t here?  I see mmap() as a tool to handle a\n> lot more data than a single malloc() typically would ;-) so previous\n> mistakes by other people would not be a good excuse.\n\nMaybe; and ...\n\n\n>> +\tif (limit == -1) {\n>> +\t\tconst char *env = getenv(\"GIT_MMAP_LIMIT\");\n>> +\t\tlimit = env ? atoi(env) * 1024 : 0;\n\n... this should then be changed to atol(env), and ... \n\n\n>> +\t}\n>> +\tif (limit && length > limit)\n>> +\t\tdie(\"attempting to mmap %\"PRIuMAX\" over limit %d\",\n>> +\t\t    (intmax_t)length, limit);\n\n... here PRIuMAX and (uintmax_t); (uintmax_t) also for length.\n\nI'll fix it and also GIT_ALLOC_LIMIT.\n\n\tSteffen\n\n\n>> +}\n>> +\n>> void *xmmap(void *start, size_t length,\n>> \tint prot, int flags, int fd, off_t offset)\n>> {\n>> -\tvoid *ret = mmap(start, length, prot, flags, fd, offset);\n>> +\tvoid *ret;\n>> +\n>> +\tmmap_limit_check(length);\n>> +\tret = mmap(start, length, prot, flags, fd, offset);\n>> \tif (ret == MAP_FAILED) {\n>> \t\tif (!length)\n>> \t\t\treturn NULL;\n"},{"id":"248123","messageId":"xmqqmwawpldt.fsf@gitster.dls.corp.google.com","threadId":"37392","inReplyTo":"0342479D-7C9F-42E6-9B79-745AEDE57EB5@zib.de","subject":"Re: [PATCH v3 2/3] Introduce GIT_MMAP_LIMIT to allow testing expected mmap size","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-22T15:57:50Z","receivedAt":"2014-08-22T15:57:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska <prohaska@zib.de> writes:\n\n>>> +\tif (limit == -1) {\n>>> +\t\tconst char *env = getenv(\"GIT_MMAP_LIMIT\");\n>>> +\t\tlimit = env ? atoi(env) * 1024 : 0;\n>\n> ... this should then be changed to atol(env), and ... \n\nIn the real codepath (not debugging aid like this) we try to avoid\natoi/atol so that we can catch errors like feeding \"123Q\" and\nparsing it as 123.\n"},{"id":"248127","messageId":"xmqqiolkpjtq.fsf@gitster.dls.corp.google.com","threadId":"37392","inReplyTo":"xmqqmwawpldt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/3] Introduce GIT_MMAP_LIMIT to allow testing expected mmap size","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-22T16:31:29Z","receivedAt":"2014-08-22T16:31:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Steffen Prohaska <prohaska@zib.de> writes:\n>\n>>>> +\tif (limit == -1) {\n>>>> +\t\tconst char *env = getenv(\"GIT_MMAP_LIMIT\");\n>>>> +\t\tlimit = env ? atoi(env) * 1024 : 0;\n>>\n>> ... this should then be changed to atol(env), and ... \n>\n> In the real codepath (not debugging aid like this) we try to avoid\n> atoi/atol so that we can catch errors like feeding \"123Q\" and\n> parsing it as 123.\n\nSorry for hitting <SEND> by mistake before finishing the paragraph,\nwhich should have concluded with:\n\n    But it is OK to be loose in an debugging aid.  If I were doing\n    this code, I actually would call git_parse_ulong() and not\n    define it in terms of kilobytes, though.\n"},{"id":"248205","messageId":"E4460202-E37A-498E-8062-DCCD9AE8E38C@zib.de","threadId":"37392","inReplyTo":"xmqqiolkpjtq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/3] Introduce GIT_MMAP_LIMIT to allow testing expected mmap size","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-08-24T16:03:24Z","receivedAt":"2014-08-24T16:03:24Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"On Aug 22, 2014, at 6:31 PM, Junio C Hamano <gitster@pobox.com> wrote:\n\n> Steffen Prohaska <prohaska@zib.de> writes:\n> \n>>>> +\tif (limit == -1) {\n>>>> +\t\tconst char *env = getenv(\"GIT_MMAP_LIMIT\");\n>>>> +\t\tlimit = env ? atoi(env) * 1024 : 0;\n>> \n>> ... this should then be changed to atol(env), and ... \n> \n> In the real codepath (not debugging aid like this) we try to avoid\n> atoi/atol so that we can catch errors like feeding \"123Q\" and\n> parsing it as 123.\n> \n> But it is OK to be loose in an debugging aid.  If I were doing\n> this code, I actually would call git_parse_ulong() and not\n> define it in terms of kilobytes, though.\n\nThanks for suggesting this.  I wasn't aware of git_parse_ulong().\nI think it makes very much sense to use it, even though it's only a\ntesting aid.\n\nI'll send a PATCH v5 series.\n\n\tSteffen\n"}]}