From 7697e54176ccab22ed6d4597d7256e9a1e6ff202 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Wed, 11 Dec 2013 15:02:20 -0800 Subject: [PATCH] Test cancel from indexer progress callback This adds tests that try canceling an indexer operation from within the progress callback. After writing the tests, I wanted to run this under valgrind and had a number of errors in that situation because mmap wasn't working. I added a CMake option to force emulation of mmap and consolidated the Amiga-specific code into that new place (so we don't actually need separate Amiga code now, just have to turn on -DNO_MMAP). Additionally, I made the indexer code propagate error codes more reliably than it used to. --- CMakeLists.txt | 7 ++++-- include/git2/pack.h | 3 ++- src/amiga/map.c | 48 ------------------------------------- src/indexer.c | 28 ++++++++++++---------- src/posix.c | 36 ++++++++++++++++++++++++++++ src/unix/map.c | 2 +- src/win32/map.c | 3 ++- tests/pack/indexer.c | 23 +++++++++--------- tests/pack/packbuilder.c | 25 ++++++++++++++++--- tests/valgrind-supp-mac.txt | 8 ------- 10 files changed, 95 insertions(+), 88 deletions(-) delete mode 100644 src/amiga/map.c diff --git a/CMakeLists.txt b/CMakeLists.txt index 9c19a5a79..48cbccb4e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -33,6 +33,7 @@ OPTION( ANDROID "Build for android NDK" OFF ) OPTION( USE_ICONV "Link with and use iconv library" OFF ) OPTION( USE_SSH "Link with libssh to enable SSH support" ON ) +OPTION( VALGRIND "Configure build for valgrind" OFF ) IF(${CMAKE_SYSTEM_NAME} MATCHES "Darwin") SET( USE_ICONV ON ) @@ -340,9 +341,11 @@ IF (WIN32 AND NOT CYGWIN) ADD_DEFINITIONS(-DWIN32 -D_WIN32_WINNT=0x0501) FILE(GLOB SRC_OS src/win32/*.c src/win32/*.h) ELSEIF (AMIGA) - ADD_DEFINITIONS(-DNO_ADDRINFO -DNO_READDIR_R) - FILE(GLOB SRC_OS src/amiga/*.c src/amiga/*.h) + ADD_DEFINITIONS(-DNO_ADDRINFO -DNO_READDIR_R -DNO_MMAP) ELSE() + IF (VALGRIND) + ADD_DEFINITIONS(-DNO_MMAP) + ENDIF() FILE(GLOB SRC_OS src/unix/*.c src/unix/*.h) ENDIF() FILE(GLOB SRC_GIT2 src/*.c src/*.h src/transports/*.c src/transports/*.h src/xdiff/*.c src/xdiff/*.h) diff --git a/include/git2/pack.h b/include/git2/pack.h index 88a2716bb..11bb559d8 100644 --- a/include/git2/pack.h +++ b/include/git2/pack.h @@ -52,7 +52,7 @@ typedef enum { GIT_PACKBUILDER_ADDING_OBJECTS = 0, GIT_PACKBUILDER_DELTAFICATION = 1, } git_packbuilder_stage_t; - + /** * Initialize a new packbuilder * @@ -143,6 +143,7 @@ GIT_EXTERN(int) git_packbuilder_write( GIT_EXTERN(const git_oid *) git_packbuilder_hash(git_packbuilder *pb); typedef int (*git_packbuilder_foreach_cb)(void *buf, size_t size, void *payload); + /** * Create the new pack and pass each object to the callback * diff --git a/src/amiga/map.c b/src/amiga/map.c deleted file mode 100644 index 0ba7995c6..000000000 --- a/src/amiga/map.c +++ /dev/null @@ -1,48 +0,0 @@ -/* - * Copyright (C) the libgit2 contributors. All rights reserved. - * - * This file is part of libgit2, distributed under the GNU GPL v2 with - * a Linking Exception. For full terms see the included COPYING file. - */ -#include - -#ifndef GIT_WIN32 - -#include "posix.h" -#include "map.h" -#include - -int p_mmap(git_map *out, size_t len, int prot, int flags, int fd, git_off_t offset) -{ - GIT_MMAP_VALIDATE(out, len, prot, flags); - - out->data = NULL; - out->len = 0; - - if ((prot & GIT_PROT_WRITE) && ((flags & GIT_MAP_TYPE) == GIT_MAP_SHARED)) { - giterr_set(GITERR_OS, "Trying to map shared-writeable"); - return -1; - } - - out->data = malloc(len); - GITERR_CHECK_ALLOC(out->data); - - if ((p_lseek(fd, offset, SEEK_SET) < 0) || ((size_t)p_read(fd, out->data, len) != len)) { - giterr_set(GITERR_OS, "mmap emulation failed"); - return -1; - } - - out->len = len; - return 0; -} - -int p_munmap(git_map *map) -{ - assert(map != NULL); - free(map->data); - - return 0; -} - -#endif - diff --git a/src/indexer.c b/src/indexer.c index 88897d07d..718c69814 100644 --- a/src/indexer.c +++ b/src/indexer.c @@ -441,8 +441,8 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran processed = stats->indexed_objects; - if (git_filebuf_write(&idx->pack_file, data, size) < 0) - return -1; + if ((error = git_filebuf_write(&idx->pack_file, data, size)) < 0) + return error; hash_partially(idx, data, (int)size); @@ -450,12 +450,12 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran if (idx->opened_pack) { idx->pack->mwf.size += size; } else { - if (open_pack(&idx->pack, idx->pack_file.path_lock) < 0) - return -1; + if ((error = open_pack(&idx->pack, idx->pack_file.path_lock)) < 0) + return error; idx->opened_pack = 1; mwf = &idx->pack->mwf; - if (git_mwindow_file_register(&idx->pack->mwf) < 0) - return -1; + if ((error = git_mwindow_file_register(&idx->pack->mwf)) < 0) + return error; } if (!idx->parsed_header) { @@ -464,8 +464,8 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran if ((unsigned)idx->pack->mwf.size < sizeof(struct git_pack_header)) return 0; - if (parse_header(&idx->hdr, idx->pack) < 0) - return -1; + if ((error = parse_header(&idx->hdr, idx->pack)) < 0) + return error; idx->parsed_header = 1; idx->nr_objects = ntohl(hdr->hdr_entries); @@ -503,6 +503,7 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran /* As the file grows any windows we try to use will be out of date */ git_mwindow_free_all(mwf); + while (processed < idx->nr_objects) { git_packfile_stream *stream = &idx->stream; git_off_t entry_start = idx->off; @@ -520,7 +521,7 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran return 0; } if (error < 0) - return error; + goto on_error; git_mwindow_close(&w); idx->entry_start = entry_start; @@ -533,7 +534,7 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran return 0; } if (error < 0) - return error; + goto on_error; idx->have_delta = 1; } else { @@ -542,9 +543,10 @@ int git_indexer_append(git_indexer *idx, const void *data, size_t size, git_tran } idx->have_stream = 1; - if (git_packfile_stream_open(stream, idx->pack, idx->off) < 0) - goto on_error; + error = git_packfile_stream_open(stream, idx->pack, idx->off); + if (error < 0) + goto on_error; } if (idx->have_delta) { @@ -858,7 +860,7 @@ int git_indexer_commit(git_indexer *idx, git_transfer_progress *stats) /* Test for this before resolve_deltas(), as it plays with idx->off */ if (idx->off < idx->pack->mwf.size - 20) { - giterr_set(GITERR_INDEXER, "unexpected data at the end of the pack"); + giterr_set(GITERR_INDEXER, "Unexpected data at the end of the pack"); return -1; } diff --git a/src/posix.c b/src/posix.c index b75109b83..525785f35 100644 --- a/src/posix.c +++ b/src/posix.c @@ -203,4 +203,40 @@ int p_write(git_file fd, const void *buf, size_t cnt) return 0; } +#ifdef NO_MMAP +#include "map.h" + +int p_mmap(git_map *out, size_t len, int prot, int flags, int fd, git_off_t offset) +{ + GIT_MMAP_VALIDATE(out, len, prot, flags); + + out->data = NULL; + out->len = 0; + + if ((prot & GIT_PROT_WRITE) && ((flags & GIT_MAP_TYPE) == GIT_MAP_SHARED)) { + giterr_set(GITERR_OS, "Trying to map shared-writeable"); + return -1; + } + + out->data = malloc(len); + GITERR_CHECK_ALLOC(out->data); + + if ((p_lseek(fd, offset, SEEK_SET) < 0) || ((size_t)p_read(fd, out->data, len) != len)) { + giterr_set(GITERR_OS, "mmap emulation failed"); + return -1; + } + + out->len = len; + return 0; +} + +int p_munmap(git_map *map) +{ + assert(map != NULL); + free(map->data); + + return 0; +} + +#endif diff --git a/src/unix/map.c b/src/unix/map.c index 7de99c99d..e62ab3e76 100644 --- a/src/unix/map.c +++ b/src/unix/map.c @@ -6,7 +6,7 @@ */ #include -#ifndef GIT_WIN32 +#if !defined(GIT_WIN32) && !defined(NO_MMAP) #include "map.h" #include diff --git a/src/win32/map.c b/src/win32/map.c index 44c6c4e2e..902ea3994 100644 --- a/src/win32/map.c +++ b/src/win32/map.c @@ -8,6 +8,7 @@ #include "map.h" #include +#ifndef NO_MMAP static DWORD get_page_size(void) { @@ -112,4 +113,4 @@ int p_munmap(git_map *map) return error; } - +#endif diff --git a/tests/pack/indexer.c b/tests/pack/indexer.c index 07963a9e7..084f8e666 100644 --- a/tests/pack/indexer.c +++ b/tests/pack/indexer.c @@ -11,7 +11,7 @@ * This is a packfile with three objects. The second is a delta which * depends on the third, which is also a delta. */ -unsigned char out_of_order_pack[] = { +static const unsigned char out_of_order_pack[] = { 0x50, 0x41, 0x43, 0x4b, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x03, 0x32, 0x78, 0x9c, 0x63, 0x67, 0x00, 0x00, 0x00, 0x10, 0x00, 0x08, 0x76, 0xe6, 0x8f, 0xe8, 0x12, 0x9b, 0x54, 0x6b, 0x10, 0x1a, 0xee, 0x95, 0x10, @@ -23,13 +23,13 @@ unsigned char out_of_order_pack[] = { 0x19, 0x87, 0x58, 0x80, 0x61, 0x09, 0x9a, 0x33, 0xca, 0x7a, 0x31, 0x92, 0x6f, 0xae, 0x66, 0x75 }; -unsigned int out_of_order_pack_len = 112; +static const unsigned int out_of_order_pack_len = 112; /* * Packfile with two objects. The second is a delta against an object * which is not in the packfile */ -unsigned char thin_pack[] = { +static const unsigned char thin_pack[] = { 0x50, 0x41, 0x43, 0x4b, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x02, 0x32, 0x78, 0x9c, 0x63, 0x67, 0x00, 0x00, 0x00, 0x10, 0x00, 0x08, 0x76, 0xe6, 0x8f, 0xe8, 0x12, 0x9b, 0x54, 0x6b, 0x10, 0x1a, 0xee, 0x95, 0x10, @@ -38,18 +38,19 @@ unsigned char thin_pack[] = { 0x3a, 0x6f, 0x39, 0xd1, 0xfe, 0x66, 0x68, 0x6b, 0xa5, 0xe5, 0xe2, 0x97, 0xac, 0x94, 0x6c, 0x76, 0x0b, 0x04 }; -unsigned int thin_pack_len = 78; +static const unsigned int thin_pack_len = 78; -unsigned char base_obj[] = { 07, 076 }; -unsigned int base_obj_len = 2; +static const unsigned char base_obj[] = { 07, 076 }; +static const unsigned int base_obj_len = 2; void test_pack_indexer__out_of_order(void) { - git_indexer *idx; - git_transfer_progress stats; + git_indexer *idx = 0; + git_transfer_progress stats = { 0 }; cl_git_pass(git_indexer_new(&idx, ".", 0, NULL, NULL, NULL)); - cl_git_pass(git_indexer_append(idx, out_of_order_pack, out_of_order_pack_len, &stats)); + cl_git_pass(git_indexer_append( + idx, out_of_order_pack, out_of_order_pack_len, &stats)); cl_git_pass(git_indexer_commit(idx, &stats)); cl_assert_equal_i(stats.total_objects, 3); @@ -61,8 +62,8 @@ void test_pack_indexer__out_of_order(void) void test_pack_indexer__fix_thin(void) { - git_indexer *idx; - git_transfer_progress stats; + git_indexer *idx = NULL; + git_transfer_progress stats = { 0 }; git_repository *repo; git_odb *odb; git_oid id, should_id; diff --git a/tests/pack/packbuilder.c b/tests/pack/packbuilder.c index 1ae2322a5..53db81828 100644 --- a/tests/pack/packbuilder.c +++ b/tests/pack/packbuilder.c @@ -12,6 +12,7 @@ static git_packbuilder *_packbuilder; static git_indexer *_indexer; static git_vector _commits; static int _commits_is_initialized; +static git_transfer_progress _stats; void test_pack_packbuilder__initialize(void) { @@ -20,6 +21,7 @@ void test_pack_packbuilder__initialize(void) cl_git_pass(git_packbuilder_new(&_packbuilder, _repo)); cl_git_pass(git_vector_init(&_commits, 0, NULL)); _commits_is_initialized = 1; + memset(&_stats, 0, sizeof(_stats)); } void test_pack_packbuilder__cleanup(void) @@ -184,11 +186,10 @@ void test_pack_packbuilder__permissions_readwrite(void) test_write_pack_permission(0666, 0666); } -static git_transfer_progress stats; static int foreach_cb(void *buf, size_t len, void *payload) { git_indexer *idx = (git_indexer *) payload; - cl_git_pass(git_indexer_append(idx, buf, len, &stats)); + cl_git_pass(git_indexer_append(idx, buf, len, &_stats)); return 0; } @@ -199,6 +200,24 @@ void test_pack_packbuilder__foreach(void) seed_packbuilder(); cl_git_pass(git_indexer_new(&idx, ".", 0, NULL, NULL, NULL)); cl_git_pass(git_packbuilder_foreach(_packbuilder, foreach_cb, idx)); - cl_git_pass(git_indexer_commit(idx, &stats)); + cl_git_pass(git_indexer_commit(idx, &_stats)); + git_indexer_free(idx); +} + +static int foreach_cancel_cb(void *buf, size_t len, void *payload) +{ + git_indexer *idx = (git_indexer *)payload; + cl_git_pass(git_indexer_append(idx, buf, len, &_stats)); + return (_stats.total_objects > 2) ? -1111 : 0; +} + +void test_pack_packbuilder__foreach_with_cancel(void) +{ + git_indexer *idx; + + seed_packbuilder(); + cl_git_pass(git_indexer_new(&idx, ".", 0, NULL, NULL, NULL)); + cl_git_fail_with( + git_packbuilder_foreach(_packbuilder, foreach_cancel_cb, idx), -1111); git_indexer_free(idx); } diff --git a/tests/valgrind-supp-mac.txt b/tests/valgrind-supp-mac.txt index 99833d091..0cdc975fa 100644 --- a/tests/valgrind-supp-mac.txt +++ b/tests/valgrind-supp-mac.txt @@ -102,14 +102,6 @@ ... fun:ssl23_connect } -{ - mac-ssl-uninitialized-4 - Memcheck:Param - ... - obj:/usr/lib/libcrypto.0.9.8.dylib - ... - fun:ssl23_connect -} { mac-ssl-leak-1 Memcheck:Leak