This moves the common setup code of about 5 different callers into one method. Error messages will now be common and shared in all places; several paths did not have any messages at all before. In addition, we now pick an ideal block size for the archive read based off the larger value of our default buffer size or the st.st_blksize field. For a filesystem such as NFS, this is often much larger than the default 8192- values such as 32768 and 131072 are common. Signed-off-by: Dan McGee <dan@archlinux.org> --- This is more academic than anything, but it does reduce all the archive setup boilerplate code we had which is probably a win in itself. I also caught a few resource leaks in error paths looking at this, whether that be unclosed archive objects, file descriptors, etc. configure.ac | 1 + lib/libalpm/add.c | 22 ++++------- lib/libalpm/be_package.c | 70 +++++++++++++++++----------------- lib/libalpm/be_sync.c | 23 +---------- lib/libalpm/util.c | 95 +++++++++++++++++++++++++++++++++++----------- lib/libalpm/util.h | 3 + 6 files changed, 123 insertions(+), 91 deletions(-) diff --git a/configure.ac b/configure.ac index c23da75..7fe1af2 100644 --- a/configure.ac +++ b/configure.ac @@ -201,6 +201,7 @@ AC_CHECK_FUNCS([dup2 getcwd geteuid getmntinfo gettimeofday memmove memset \ mkdir realpath regcomp rmdir setenv setlocale strcasecmp \ strchr strcspn strdup strerror strndup strrchr strsep strstr \ strtol swprintf tcflush wcwidth uname]) +AC_CHECK_MEMBERS([struct stat.st_blksize],,,[[#include <sys/stat.h>]]) # For the diskspace code FS_STATS_TYPE diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 44b852f..78615bb 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -525,23 +525,14 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, if(!(trans->flags & ALPM_TRANS_FLAG_DBONLY)) { struct archive *archive; struct archive_entry *entry; - int cwdfd; + struct stat buf; + int fd, cwdfd; _alpm_log(handle, ALPM_LOG_DEBUG, "extracting files\n"); - if((archive = archive_read_new()) == NULL) { - handle->pm_errno = ALPM_ERR_LIBARCHIVE; - ret = -1; - goto cleanup; - } - - archive_read_support_compression_all(archive); - archive_read_support_format_all(archive); - - _alpm_log(handle, ALPM_LOG_DEBUG, "archive: %s\n", pkgfile); - if(archive_read_open_filename(archive, pkgfile, - ALPM_BUFFER_SIZE) != ARCHIVE_OK) { - handle->pm_errno = ALPM_ERR_PKG_OPEN; + fd = _alpm_open_archive(db->handle, pkgfile, &buf, + &archive, ALPM_ERR_PKG_OPEN); + if(fd < 0) { ret = -1; goto cleanup; } @@ -556,6 +547,8 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, if(chdir(handle->root) != 0) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not change directory to %s (%s)\n"), handle->root, strerror(errno)); + archive_read_finish(archive); + CLOSE(fd); ret = -1; goto cleanup; } @@ -597,6 +590,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, errors += extract_single_file(handle, archive, entry, newpkg, oldpkg); } archive_read_finish(archive); + CLOSE(fd); /* restore the old cwd if we have it */ if(cwdfd >= 0) { diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index 90a1972..4f530e0 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -40,6 +40,11 @@ #include "package.h" #include "deps.h" /* _alpm_splitdep */ +struct package_changelog { + struct archive *archive; + int fd; +}; + /** * Open a package changelog for reading. Similar to fopen in functionality, * except that the returned 'file stream' is from an archive. @@ -50,31 +55,38 @@ static void *_package_changelog_open(alpm_pkg_t *pkg) { ASSERT(pkg != NULL, return NULL); - struct archive *archive = NULL; + struct package_changelog *changelog; + struct archive *archive; struct archive_entry *entry; const char *pkgfile = pkg->origin_data.file; + struct stat buf; + int fd; - if((archive = archive_read_new()) == NULL) { - RET_ERR(pkg->handle, ALPM_ERR_LIBARCHIVE, NULL); - } - - archive_read_support_compression_all(archive); - archive_read_support_format_all(archive); - - if(archive_read_open_filename(archive, pkgfile, - ALPM_BUFFER_SIZE) != ARCHIVE_OK) { - RET_ERR(pkg->handle, ALPM_ERR_PKG_OPEN, NULL); + fd = _alpm_open_archive(pkg->handle, pkgfile, &buf, + &archive, ALPM_ERR_PKG_OPEN); + if(fd < 0) { + return NULL; } while(archive_read_next_header(archive, &entry) == ARCHIVE_OK) { const char *entry_name = archive_entry_pathname(entry); if(strcmp(entry_name, ".CHANGELOG") == 0) { - return archive; + changelog = malloc(sizeof(struct package_changelog)); + if(!changelog) { + pkg->handle->pm_errno = ALPM_ERR_MEMORY; + archive_read_finish(archive); + CLOSE(fd); + return NULL; + } + changelog->archive = archive; + changelog->fd = fd; + return changelog; } } /* we didn't find a changelog */ archive_read_finish(archive); + CLOSE(fd); errno = ENOENT; return NULL; @@ -92,7 +104,8 @@ static void *_package_changelog_open(alpm_pkg_t *pkg) static size_t _package_changelog_read(void *ptr, size_t size, const alpm_pkg_t UNUSED *pkg, void *fp) { - ssize_t sret = archive_read_data((struct archive *)fp, ptr, size); + struct package_changelog *changelog = fp; + ssize_t sret = archive_read_data(changelog->archive, ptr, size); /* Report error (negative values) */ if(sret < 0) { RET_ERR(pkg->handle, ALPM_ERR_LIBARCHIVE, 0); @@ -110,7 +123,12 @@ static size_t _package_changelog_read(void *ptr, size_t size, */ static int _package_changelog_close(const alpm_pkg_t UNUSED *pkg, void *fp) { - return archive_read_finish((struct archive *)fp); + int ret; + struct package_changelog *changelog = fp; + ret = archive_read_finish(changelog->archive); + CLOSE(changelog->fd); + free(changelog); + return ret; } /** Package file operations struct accessor. We implement this as a method @@ -370,24 +388,10 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, RET_ERR(handle, ALPM_ERR_WRONG_ARGS, NULL); } - /* try to create an archive object to read in the package */ - if((archive = archive_read_new()) == NULL) { - RET_ERR(handle, ALPM_ERR_LIBARCHIVE, NULL); - } - - archive_read_support_compression_all(archive); - archive_read_support_format_all(archive); - - OPEN(fd, pkgfile, O_RDONLY); - if(fd < 0 || archive_read_open_fd(archive, fd, - ALPM_BUFFER_SIZE) != ARCHIVE_OK) { - const char *err = fd < 0 ? strerror(errno) : archive_error_string(archive); - _alpm_log(handle, ALPM_LOG_ERROR, - _("could not open file %s: %s\n"), pkgfile, err); - if(fd < 0 && errno == ENOENT) { + fd = _alpm_open_archive(handle, pkgfile, &st, &archive, ALPM_ERR_PKG_OPEN); + if(fd < 0) { + if(errno == ENOENT) { handle->pm_errno = ALPM_ERR_PKG_NOT_FOUND; - } else { - handle->pm_errno = ALPM_ERR_PKG_OPEN; } goto error; } @@ -397,10 +401,6 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, handle->pm_errno = ALPM_ERR_MEMORY; goto error; } - if(fstat(fd, &st) != 0) { - handle->pm_errno = ALPM_ERR_PKG_OPEN; - goto error; - } STRDUP(newpkg->filename, pkgfile, handle->pm_errno = ALPM_ERR_MEMORY; goto error); newpkg->size = st.st_size; diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index aa26002..54c4f87 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -433,26 +433,9 @@ static int sync_db_populate(alpm_db_t *db) return -1; } - if((archive = archive_read_new()) == NULL) { - RET_ERR(db->handle, ALPM_ERR_LIBARCHIVE, -1); - } - - archive_read_support_compression_all(archive); - archive_read_support_format_all(archive); - - _alpm_log(db->handle, ALPM_LOG_DEBUG, - "opening database archive %s\n", dbpath); - OPEN(fd, dbpath, O_RDONLY); - if(fd < 0 || archive_read_open_fd(archive, fd, - ALPM_BUFFER_SIZE) != ARCHIVE_OK) { - const char *err = fd < 0 ? strerror(errno) : archive_error_string(archive); - _alpm_log(db->handle, ALPM_LOG_ERROR, - _("could not open file %s: %s\n"), dbpath, err); - db->handle->pm_errno = ALPM_ERR_DB_OPEN; - goto cleanup; - } - if(fstat(fd, &buf) != 0) { - db->handle->pm_errno = ALPM_ERR_DB_OPEN; + fd = _alpm_open_archive(db->handle, dbpath, &buf, + &archive, ALPM_ERR_DB_OPEN); + if(fd < 0) { goto cleanup; } est_count = estimate_package_count(&buf, archive); diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 5070cb9..ffd835a 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -233,6 +233,64 @@ size_t _alpm_strip_newline(char *str) /* Compression functions */ /** + * Open an archive for reading and perform the necessary boilerplate. + * This takes care of creating the libarchive 'archive' struct, setting up + * compression and format options, opening a file descriptor, setting up the + * buffer size, and performing a stat on the path once opened. + * @param handle the context handle + * @param path the path of the archive to open + * @param buf space for a stat buffer for the given path + * @param archive pointer to place the created archive object + * @param error error code to set on failure to open archive + * @return -1 on failure, >=0 file descriptor on success + */ +int _alpm_open_archive(alpm_handle_t *handle, const char *path, + struct stat *buf, struct archive **archive, enum _alpm_errno_t error) +{ + int fd; + size_t bufsize = ALPM_BUFFER_SIZE; + + if((*archive = archive_read_new()) == NULL) { + RET_ERR(handle, ALPM_ERR_LIBARCHIVE, -1); + } + + archive_read_support_compression_all(*archive); + archive_read_support_format_all(*archive); + + _alpm_log(handle, ALPM_LOG_DEBUG, "opening archive %s\n", path); + OPEN(fd, path, O_RDONLY); + if(fd < 0) { + _alpm_log(handle, ALPM_LOG_ERROR, + _("could not open file %s: %s\n"), path, strerror(errno)); + archive_read_finish(*archive); + RET_ERR(handle, error, -1); + } + + if(fstat(fd, buf) != 0) { + _alpm_log(handle, ALPM_LOG_ERROR, + _("could not stat file %s: %s\n"), path, strerror(errno)); + archive_read_finish(*archive); + CLOSE(fd); + RET_ERR(handle, error, -1); + } +#ifdef HAVE_STRUCT_STAT_ST_BLKSIZE + if(buf->st_blksize > ALPM_BUFFER_SIZE) { + bufsize = buf->st_blksize; + } +#endif + + if(archive_read_open_fd(*archive, fd, bufsize) != ARCHIVE_OK) { + _alpm_log(handle, ALPM_LOG_ERROR, _("could not open file %s: %s\n"), + path, archive_error_string(*archive)); + archive_read_finish(*archive); + CLOSE(fd); + RET_ERR(handle, error, -1); + } + + return fd; +} + +/** * @brief Unpack a specific file in an archive. * * @param handle the context handle @@ -259,34 +317,26 @@ int _alpm_unpack_single(alpm_handle_t *handle, const char *archive, * @brief Unpack a list of files in an archive. * * @param handle the context handle - * @param archive the archive to unpack + * @param path the archive to unpack * @param prefix where to extract the files * @param list a list of files within the archive to unpack or NULL for all * @param breakfirst break after the first entry found * * @return 0 on success, 1 on failure */ -int _alpm_unpack(alpm_handle_t *handle, const char *archive, const char *prefix, +int _alpm_unpack(alpm_handle_t *handle, const char *path, const char *prefix, alpm_list_t *list, int breakfirst) { int ret = 0; mode_t oldmask; - struct archive *_archive; + struct archive *archive; struct archive_entry *entry; - int cwdfd; - - if((_archive = archive_read_new()) == NULL) { - RET_ERR(handle, ALPM_ERR_LIBARCHIVE, 1); - } - - archive_read_support_compression_all(_archive); - archive_read_support_format_all(_archive); + struct stat buf; + int fd, cwdfd; - if(archive_read_open_filename(_archive, archive, - ALPM_BUFFER_SIZE) != ARCHIVE_OK) { - _alpm_log(handle, ALPM_LOG_ERROR, _("could not open file %s: %s\n"), archive, - archive_error_string(_archive)); - RET_ERR(handle, ALPM_ERR_PKG_OPEN, 1); + fd = _alpm_open_archive(handle, path, &buf, &archive, ALPM_ERR_PKG_OPEN); + if(fd < 0) { + return 1; } oldmask = umask(0022); @@ -305,7 +355,7 @@ int _alpm_unpack(alpm_handle_t *handle, const char *archive, const char *prefix, goto cleanup; } - while(archive_read_next_header(_archive, &entry) == ARCHIVE_OK) { + while(archive_read_next_header(archive, &entry) == ARCHIVE_OK) { const char *entryname; mode_t mode; @@ -321,7 +371,7 @@ int _alpm_unpack(alpm_handle_t *handle, const char *archive, const char *prefix, char *found = alpm_list_find_str(list, entry_prefix); free(entry_prefix); if(!found) { - if(archive_read_data_skip(_archive) != ARCHIVE_OK) { + if(archive_read_data_skip(archive) != ARCHIVE_OK) { ret = 1; goto cleanup; } @@ -339,14 +389,14 @@ int _alpm_unpack(alpm_handle_t *handle, const char *archive, const char *prefix, } /* Extract the archive entry. */ - int readret = archive_read_extract(_archive, entry, 0); + int readret = archive_read_extract(archive, entry, 0); if(readret == ARCHIVE_WARN) { /* operation succeeded but a non-critical error was encountered */ _alpm_log(handle, ALPM_LOG_WARNING, _("warning given when extracting %s (%s)\n"), - entryname, archive_error_string(_archive)); + entryname, archive_error_string(archive)); } else if(readret != ARCHIVE_OK) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not extract %s (%s)\n"), - entryname, archive_error_string(_archive)); + entryname, archive_error_string(archive)); ret = 1; goto cleanup; } @@ -358,7 +408,8 @@ int _alpm_unpack(alpm_handle_t *handle, const char *archive, const char *prefix, cleanup: umask(oldmask); - archive_read_finish(_archive); + archive_read_finish(archive); + CLOSE(fd); if(cwdfd >= 0) { if(fchdir(cwdfd) != 0) { _alpm_log(handle, ALPM_LOG_ERROR, diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h index df16543..e7e15be 100644 --- a/lib/libalpm/util.h +++ b/lib/libalpm/util.h @@ -117,6 +117,9 @@ int _alpm_makepath_mode(const char *path, mode_t mode); int _alpm_copyfile(const char *src, const char *dest); char *_alpm_strtrim(char *str); size_t _alpm_strip_newline(char *str); + +int _alpm_open_archive(alpm_handle_t *handle, const char *path, + struct stat *buf, struct archive **archive, enum _alpm_errno_t error); int _alpm_unpack_single(alpm_handle_t *handle, const char *archive, const char *prefix, const char *filename); int _alpm_unpack(alpm_handle_t *handle, const char *archive, const char *prefix, -- 1.7.7.3