From 616a358dc79fec4c4453deb8f997e789d5fd64c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 2 May 2013 13:09:51 +0200 Subject: [PATCH] Prefer binary realloc to self-managed buffers Use erlang's allocator and simplify the code by writing into the buffer we're going to give back anyway. Also introduce a function to NUL-terminate iolist buffers, which can avoid memory copying. --- c_src/commit.c | 1 - c_src/geef.c | 10 +++++++ c_src/geef.h | 3 +++ c_src/reference.c | 29 +++++--------------- c_src/repository.c | 67 +++++++++------------------------------------- c_src/tree.c | 12 ++------- 6 files changed, 33 insertions(+), 89 deletions(-) diff --git a/c_src/commit.c b/c_src/commit.c index 9114288..faf0a33 100644 --- a/c_src/commit.c +++ b/c_src/commit.c @@ -40,7 +40,6 @@ geef_commit_tree(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) term_obj = enif_make_resource(env, obj); enif_release_resource(obj); - return enif_make_tuple3(env, atoms.ok, atoms.tree, term_obj); } diff --git a/c_src/geef.c b/c_src/geef.c index 210077b..de30b0e 100644 --- a/c_src/geef.c +++ b/c_src/geef.c @@ -123,6 +123,16 @@ geef_oom(ErlNifEnv *env) return enif_make_tuple2(env, atoms.error, atoms.enomem); } +int geef_terminate_binary(ErlNifBinary *bin) +{ + if (!enif_realloc_binary(bin, bin->size + 1)) + return 0; + + bin->data[bin->size - 1] = '\0'; + + return 1; +} + static ErlNifFunc geef_funcs[] = { {"repository_init", 2, geef_repository_init}, diff --git a/c_src/geef.h b/c_src/geef.h index acd7808..0479081 100644 --- a/c_src/geef.h +++ b/c_src/geef.h @@ -29,4 +29,7 @@ typedef struct { extern geef_atoms atoms; +/** NUL-terminate a binary */ +int geef_terminate_binary(ErlNifBinary *bin); + #endif diff --git a/c_src/reference.c b/c_src/reference.c index c408a20..4a0901e 100644 --- a/c_src/reference.c +++ b/c_src/reference.c @@ -49,7 +49,6 @@ geef_reference_lookup(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) git_reference *ref; ErlNifBinary bin; ERL_NIF_TERM term_ref; - char *name; int error; if (!enif_get_resource(env, argv[0], geef_repository_type, (void **) &repo)) @@ -58,15 +57,10 @@ geef_reference_lookup(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) if (!enif_inspect_iolist_as_binary(env, argv[1], &bin)) return enif_make_badarg(env); - name = malloc(bin.size + 1); - if (!name) + if (!geef_terminate_binary(&bin)) return geef_oom(env); - memcpy(name, bin.data, bin.size); - name[bin.size] = '\0'; - - error = git_reference_lookup(&ref, repo->repo, name); - free(name); + error = git_reference_lookup(&ref, repo->repo, (char *)bin.data); if (error < 0) return geef_error(env); @@ -128,7 +122,6 @@ static int append_to_list(const char *name, void *payload) ERL_NIF_TERM geef_reference_glob(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { - char *glob; int error; geef_repository *repo; ErlNifBinary bin; @@ -140,18 +133,13 @@ geef_reference_glob(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) if (!enif_inspect_iolist_as_binary(env, argv[1], &bin)) return enif_make_badarg(env); - glob = malloc(bin.size + 1); - if (!glob) + if (!geef_terminate_binary(&bin)) return geef_oom(env); - memcpy(glob, bin.data, bin.size); - glob[bin.size] = '\0'; - data.env = env; data.list = enif_make_list(env, 0); - error = git_reference_foreach_glob(repo->repo, glob, GIT_REF_LISTALL, append_to_list, &data); - free(glob); + error = git_reference_foreach_glob(repo->repo, (char *) bin.data, GIT_REF_LISTALL, append_to_list, &data); if (error < 0) return geef_error(env); @@ -195,7 +183,6 @@ geef_reference_to_id(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { geef_repository *repo; ErlNifBinary bin; - char *name; git_oid id; if (!enif_get_resource(env, argv[0], geef_repository_type, (void **) &repo)) @@ -204,14 +191,10 @@ geef_reference_to_id(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) if (!enif_inspect_iolist_as_binary(env, argv[1], &bin)) return enif_make_badarg(env); - name = malloc(bin.size + 1); - if (!name) + if (!geef_terminate_binary(&bin)) return geef_oom(env); - memcpy(name, bin.data, bin.size); - name[bin.size] = '\0'; - - if (git_reference_name_to_id(&id, repo->repo, name) < 0) + if (git_reference_name_to_id(&id, repo->repo, (char *)bin.data) < 0) return geef_error(env); if (geef_oid_bin(&bin, &id) < 0) diff --git a/c_src/repository.c b/c_src/repository.c index 952cee1..bc0c848 100644 --- a/c_src/repository.c +++ b/c_src/repository.c @@ -20,8 +20,7 @@ void geef_odb_free(ErlNifEnv *env, void *cd) ERL_NIF_TERM geef_repository_init(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { - char *path; - int error, bare; + int bare; git_repository *repo; geef_repository *res_repo; ErlNifBinary bin; @@ -30,19 +29,12 @@ geef_repository_init(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) if (!enif_inspect_iolist_as_binary(env, argv[0], &bin)) return enif_make_badarg(env); - path = malloc(bin.size + 1); - if (!path) + if (!geef_terminate_binary(&bin)) return geef_oom(env); - memcpy(path, bin.data, bin.size); - path[bin.size] = '\0'; - bare = !enif_compare(argv[1], atoms.true); - error = git_repository_init(&repo, path, bare); - free(path); - - if (error < 0) + if (git_repository_init(&repo, (char *) bin.data, bare) < 0) return geef_error(env); res_repo = enif_alloc_resource(geef_repository_type, sizeof(geef_repository)); @@ -56,8 +48,6 @@ geef_repository_init(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) ERL_NIF_TERM geef_repository_open(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { - char *path; - int error; git_repository *repo; geef_repository *res_repo; ErlNifBinary bin; @@ -66,17 +56,10 @@ geef_repository_open(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) if (!enif_inspect_iolist_as_binary(env, argv[0], &bin)) return enif_make_badarg(env); - path = malloc(bin.size + 1); - if (!path) + if (!geef_terminate_binary(&bin)) return geef_oom(env); - memcpy(path, bin.data, bin.size); - path[bin.size] = '\0'; - - error = git_repository_open(&repo, path); - free(path); - - if (error < 0) + if (git_repository_open(&repo, (char *) bin.data) < 0) return geef_error(env); res_repo = enif_alloc_resource(geef_repository_type, sizeof(geef_repository)); @@ -90,57 +73,31 @@ geef_repository_open(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) ERL_NIF_TERM geef_repository_discover(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { - char *path, *buffer; - size_t buffer_len; - ErlNifBinary bin, res_bin; + ErlNifBinary bin, path; int error; if (!enif_inspect_iolist_as_binary(env, argv[0], &bin)) return enif_make_badarg(env); - path = malloc(bin.size + 1); - if (!path) + if (!geef_terminate_binary(&bin)) return geef_oom(env); - memcpy(path, bin.data, bin.size); - path[bin.size] = '\0'; - - buffer_len = 256; - buffer = malloc(buffer_len); - if (!buffer) + if (!enif_alloc_binary(256, &path)) return geef_oom(env); - while ((error = git_repository_discover(buffer, buffer_len, path, 0, NULL)) < 0 && + while ((error = git_repository_discover((char *) path.data, path.size,(char *) bin.data, 0, NULL)) < 0 && giterr_last()->klass == GITERR_REPOSITORY) { - char *tmp; - buffer_len *= 2; - tmp = realloc(buffer, buffer_len); - if (!tmp) { - free(path); - free(buffer); + if (!enif_realloc_binary(&path, path.size * 2)) return geef_oom(env); - } - - buffer = tmp; } - free(path); - if (error < 0) { - free(buffer); return geef_error(env); } - buffer_len = strlen(buffer); - if (!enif_alloc_binary(buffer_len, &res_bin)) { - free(buffer); - return geef_oom(env); - } + enif_realloc_binary(&path, strlen((char *) path.data)); - memcpy(res_bin.data, buffer, buffer_len); - free(buffer); - - return enif_make_tuple2(env, atoms.ok, enif_make_binary(env, &res_bin)); + return enif_make_tuple2(env, atoms.ok, enif_make_binary(env, &path)); } ERL_NIF_TERM diff --git a/c_src/tree.c b/c_src/tree.c index 7062425..6c8860d 100644 --- a/c_src/tree.c +++ b/c_src/tree.c @@ -40,7 +40,6 @@ static ERL_NIF_TERM tree_entry_to_term(ErlNifEnv *env, const git_tree_entry *ent ERL_NIF_TERM geef_tree_bypath(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { - char *path; int error; geef_object *obj; ErlNifBinary bin; @@ -52,17 +51,10 @@ geef_tree_bypath(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) if (!enif_inspect_iolist_as_binary(env, argv[1], &bin)) return enif_make_badarg(env); - path = malloc(bin.size + 1); - if (!path) + if (!geef_terminate_binary(&bin)) return geef_oom(env); - memcpy(path, bin.data, bin.size); - path[bin.size] = '\0'; - - error = git_tree_entry_bypath(&entry, (git_tree *)obj->obj, path); - free(path); - - if (error < 0) + if (git_tree_entry_bypath(&entry, (git_tree *)obj->obj, (char *) bin.data) < 0) return geef_error(env); return tree_entry_to_term(env, entry);