[pacman-dev] [PATCH 1/3] libalpm/util: two stat() related cleanups
First, use fstat() in preference to stat() since we already have an open
file handle. This also removes the need to check for a symlink as that
is not possible when a file is opened.
Next, use archive_entry_mode() rather than archive_entry_stat() as we
only use the mode portion of the stat struct and the call is much
cheaper. Also delay it until it is necessary.
Signed-off-by: Dan McGee
This gives us a bit more control and over the archive reading process,
and a bit less is done behind the scenes. It also allows us to use
fstat() in preference to stat(), which should avoid some potential race
conditions.
Some reorganization is necessary to move the stat calls after the open()
calls. Error handling and cleanup in general is also improved, as we had
several potential memory and file handle leaks before in some error
paths.
Signed-off-by: Dan McGee
Define a single BUFFER_SIZE constant in util.h and use it everywhere.
This also prepares us for libarchive 3.0, where the
ARCHIVE_DEFAULT_BYTES_PER_BLOCK constant has been taken out to the
woodshed.
16384 was pulled out of the air; I figured we can spare 16K of memory at
a time, vs. the usual used 8K before.
Signed-off-by: Dan McGee
On Wed, Oct 26, 2011 at 05:44:43PM -0500, Dan McGee wrote:
Define a single BUFFER_SIZE constant in util.h and use it everywhere. This also prepares us for libarchive 3.0, where the ARCHIVE_DEFAULT_BYTES_PER_BLOCK constant has been taken out to the woodshed.
Any particular reason we're not using BUFSIZ out of stdio.h if its available?
16384 was pulled out of the air; I figured we can spare 16K of memory at a time, vs. the usual used 8K before.
Signed-off-by: Dan McGee
--- I wrote this on master, but it might be wise to port it to maint and be proactive so old releases don't suddenly break and need patching with a new libarchive. Pretty low-risk here.
lib/libalpm/add.c | 2 +- lib/libalpm/be_package.c | 6 ++---- lib/libalpm/be_sync.c | 3 +-- lib/libalpm/util.c | 13 ++++--------- lib/libalpm/util.h | 3 +++ 5 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 2ef4178..e6ac4e1 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -537,7 +537,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg,
_alpm_log(handle, ALPM_LOG_DEBUG, "archive: %s\n", newpkg->origin_data.file); if(archive_read_open_filename(archive, newpkg->origin_data.file, - ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { + BUFFER_SIZE) != ARCHIVE_OK) { handle->pm_errno = ALPM_ERR_PKG_OPEN; ret = -1; goto cleanup; diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index 36666df..3d76afe 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -61,8 +61,7 @@ static void *_package_changelog_open(alpm_pkg_t *pkg) archive_read_support_compression_all(archive); archive_read_support_format_all(archive);
- if(archive_read_open_filename(archive, pkgfile, - ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { + if(archive_read_open_filename(archive, pkgfile, BUFFER_SIZE) != ARCHIVE_OK) { RET_ERR(pkg->handle, ALPM_ERR_PKG_OPEN, NULL); }
@@ -382,8 +381,7 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, fd = open(pkgfile, O_RDONLY); } while(fd == -1 && errno == EINTR);
- if(fd < 0 || archive_read_open_fd(archive, fd, - ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { + if(fd < 0 || archive_read_open_fd(archive, fd, 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); diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 24785f1..eff1ca0 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -446,8 +446,7 @@ static int sync_db_populate(alpm_db_t *db) fd = open(dbpath, O_RDONLY); } while(fd == -1 && errno == EINTR);
- if(fd < 0 || archive_read_open_fd(archive, fd, - ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { + if(fd < 0 || archive_read_open_fd(archive, fd, 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); diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index af9cf42..a5ed977 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -129,8 +129,6 @@ int _alpm_makepath_mode(const char *path, mode_t mode) return ret; }
-#define CPBUFSIZE 8 * 1024 - int _alpm_copyfile(const char *src, const char *dest) { FILE *in, *out; @@ -148,10 +146,10 @@ int _alpm_copyfile(const char *src, const char *dest) return 1; }
- CALLOC(buf, (size_t)CPBUFSIZE, (size_t)1, ret = 1; goto cleanup;); + MALLOC(buf, (size_t)BUFFER_SIZE, ret = 1; goto cleanup;);
/* do the actual file copy */ - while((len = fread(buf, 1, CPBUFSIZE, in))) { + while((len = fread(buf, 1, BUFFER_SIZE, in))) { size_t nwritten = 0; nwritten = fwrite(buf, 1, len, out); if((nwritten != len) || ferror(out)) { @@ -172,7 +170,7 @@ int _alpm_copyfile(const char *src, const char *dest) cleanup: fclose(in); fclose(out); - FREE(buf); + free(buf); return ret; }
@@ -285,8 +283,7 @@ int _alpm_unpack(alpm_handle_t *handle, const char *archive, const char *prefix, archive_read_support_compression_all(_archive); archive_read_support_format_all(_archive);
- if(archive_read_open_filename(_archive, archive, - ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { + if(archive_read_open_filename(_archive, archive, 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); @@ -740,8 +737,6 @@ int _alpm_lstat(const char *path, struct stat *buf) }
#ifdef HAVE_LIBSSL -#define BUFFER_SIZE 8192 - static int md5_file(const char *path, unsigned char output[16]) { FILE *f; diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h index 400a4ee..2514946 100644 --- a/lib/libalpm/util.h +++ b/lib/libalpm/util.h @@ -74,6 +74,9 @@
#define CHECK_HANDLE(handle, action) do { if(!(handle)) { action; } (handle)->pm_errno = 0; } while(0)
+/** Standard buffer size used throughout the library. */ +#define BUFFER_SIZE 16384 + /** * Used as a buffer/state holder for _alpm_archive_fgets(). */ -- 1.7.7
On Wed, Oct 26, 2011 at 6:16 PM, Dave Reisner
On Wed, Oct 26, 2011 at 05:44:43PM -0500, Dan McGee wrote:
Define a single BUFFER_SIZE constant in util.h and use it everywhere. This also prepares us for libarchive 3.0, where the ARCHIVE_DEFAULT_BYTES_PER_BLOCK constant has been taken out to the woodshed.
Any particular reason we're not using BUFSIZ out of stdio.h if its available?
1. It is documented only as "this is the size the functions in stdio.h use", not necessarily as "this is the buffer size user space applications should use". 2. Finding that documentation is hard. [1] 3. Any particular reason to? At least on my system it is 8192, and I guess I have no idea if that varies across systems. So yeah, I don't know. I suppose I could be convinced, and we could just ifdef a hardcoded value in there if BUFSIZ wasn't defined, but I also don't know that if that value has any sort of real magic to it. -Dan [1] The best I could find was http://www.delorie.com/gnu/docs/glibc/libc_226.html, and even that is glibc specific. Sometimes people also use BUFSIZ as the allocation size of buffers used for related purposes, such as strings used to receive a line of input with fgets (see section 12.8 Character Input). There is no particular reason to use BUFSIZ for this instead of any other integer, except that it might lead to doing I/O in chunks of an efficient size.
participants (3)
-
Dan McGee
-
Dan McGee
-
Dave Reisner