This is used to deduplicate code when using the mtree as the file list source. Signed-off-by: Florian Pritz --- lib/libalpm/be_package.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index 149f281..066c220 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -353,6 +353,23 @@ int _alpm_pkg_validate_internal(alpm_handle_t *handle, } /** + * Handle the existance of simple paths for _alpm_load_pkg_internal() + * @param pkg package to change + * @param path path to examine + * @return 0 if path doesn't match any rule, 1 if it has been handled + */ +static int handle_simple_path(alpm_pkg_t *pkg, const char *path) +{ + if(strcmp(path, ".INSTALL") == 0) { + pkg->scriptlet = 1; + } else { + return 0; + } + + return 1; +} + +/** * Load a package and create the corresponding alpm_pkg_t struct. * @param handle the context handle * @param pkgfile path to the package file @@ -423,8 +440,8 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, } config = 1; continue; - } else if(strcmp(entry_name, ".INSTALL") == 0) { - newpkg->scriptlet = 1; + } else if(handle_simple_path(newpkg, entry_name)) { + continue; } else if(*entry_name == '.') { /* for now, ignore all files starting with '.' that haven't * already been handled (for future possibilities) */ -- 1.8.5.3

This greatly speeds up file list generation times by avoiding uncompressing the whole package. pacman -S base with a deliberate file conflict: before: 9.1 seconds after: 2.2 seconds Signed-off-by: Florian Pritz --- lib/libalpm/be_package.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 153 insertions(+), 2 deletions(-) diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index 5e000bb..286b964 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -381,6 +381,8 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, struct a { const size_t files_count = pkg->files.count; alpm_file_t *current_file; + mode_t type; + size_t pathlen; const char *path; alpm_file_t *tmp; @@ -389,8 +391,26 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, struct a } pkg->files.files = tmp; + type = archive_entry_filetype(entry); path = archive_entry_pathname(entry); + pathlen = strlen(path); + + /* mtree paths don't contain a tailing slash, those we get from + * the archive directly do (expensive way) + * Other code relies on it to detect directories so add it here.*/ + if(type == AE_IFDIR && path[pathlen - 1] != '/') { + /* 2 = 1 for / + 1 for \0 */ + char *newpath = malloc(pathlen + 2); + if (!newpath) { + _alpm_alloc_fail(pathlen + 2); + return -1; + } + strcpy(newpath, path); + newpath[pathlen] = '/'; + newpath[pathlen + 1] = '\0'; + path = newpath; + } current_file = pkg->files.files + files_count; STRDUP(current_file->name, path, return -1); current_file->size = archive_entry_size(entry); @@ -400,6 +420,124 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, struct a } /** + * Generate a new file list from an mtree file and add it to the package. + * An existing file list will be free()d first. + * + * archive should point to an archive struct which is already at the + * position of the mtree's header. + * + * @param handle + * @param pkg package to add the file list to + * @param archive archive containing the mtree + * @return 0 on success, <0 on error + */ +static int build_filelist_from_mtree(alpm_handle_t *handle, alpm_pkg_t *pkg, struct archive *archive) +{ + int ret = 0; + size_t mtree_msize = 0; + size_t mtree_csize = 0; + size_t files_size = 0; // we clean up the existing array so this is fine + char *mtree_data = NULL; + struct archive *mtree; + struct archive_entry *mtree_entry = NULL; + + _alpm_log(handle, ALPM_LOG_DEBUG, "found mtree for package %s, getting file list\n", pkg->filename); + + /* throw away any files we might have already found */ + free(pkg->files.files); + pkg->files.files = NULL; + pkg->files.count = 0; + + /* create a new archive to parse the mtree and load it from archive into memory */ + // TODO: split this into a function + if((mtree = archive_read_new()) == NULL) { + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + + _alpm_archive_read_support_filter_all(mtree); + archive_read_support_format_mtree(mtree); + + // TODO: split this into a function + while(1) { + ssize_t size; + char *tmp; + + if((tmp = _alpm_realloc(mtree_data, &mtree_msize, mtree_csize)) == NULL) { + goto error; + } + mtree_data = tmp; + + size = archive_read_data(archive, mtree_data + mtree_csize, ALPM_BUFFER_SIZE); + + if(size < 0) { + _alpm_log(handle, ALPM_LOG_ERROR, _("error while reading package %s: %s\n"), + pkg->filename, archive_error_string(archive)); + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + if(size == 0) { + break; + } + + mtree_csize += size; + } + + // TODO: need wrapper function for archive_read_open_memory for old libarchive versions? + if(archive_read_open_memory(mtree, mtree_data, mtree_csize)) { + _alpm_log(handle, ALPM_LOG_ERROR, _("error while reading mtree of package %s: %s\n"), + pkg->filename, archive_error_string(mtree)); + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + + while((ret = archive_read_next_header(mtree, &mtree_entry)) == ARCHIVE_OK) { + const char *path = archive_entry_pathname(mtree_entry); + char *p = NULL; + + /* strip leading "./" from path entries */ + if(path[0] == '.' && path[1] == '/') { + path += 2; + } + + if(handle_simple_path(pkg, path)) { + continue; + } + + if(*path == '.') { + continue; + } + + /* Apparently archive_entry_copy/set_pathname doesn't like to get it's own + * pointer and produces rather strange results */ + STRDUP(p, path, goto error); + archive_entry_copy_pathname(mtree_entry, p); + + if(add_entry_to_files_list(pkg, &files_size, mtree_entry) < 0) { + free(p); + goto error; + } + free(p); + } + + if(ret != ARCHIVE_EOF && ret != ARCHIVE_OK) { /* An error occurred */ + _alpm_log(handle, ALPM_LOG_ERROR, _("error while reading mtree of package %s: %s\n"), + pkg->filename, archive_error_string(mtree)); + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + + free(mtree_data); + _alpm_archive_read_free(mtree); + _alpm_log(handle, ALPM_LOG_DEBUG, "finished mtree reading for %s\n", pkg->filename); + return 0; +error: + free(mtree_data); + _alpm_archive_read_free(mtree); + return -1; +} + +/** * Load a package and create the corresponding alpm_pkg_t struct. * @param handle the context handle * @param pkgfile path to the package file @@ -409,7 +547,7 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, struct a alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, const char *pkgfile, int full) { - int ret, fd, config = 0; + int ret, fd, config, hit_mtree = 0; struct archive *archive; struct archive_entry *entry; alpm_pkg_t *newpkg; @@ -472,10 +610,23 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, continue; } else if(handle_simple_path(newpkg, entry_name)) { continue; + } else if(full && strcmp(entry_name, ".MTREE") == 0) { + /* building the file list: cheap way + * get the filelist from the mtree file rather than scanning + * the whole archive */ + if(build_filelist_from_mtree(handle, newpkg, archive) < 0) { + goto error; + } + hit_mtree = 1; + continue; } else if(*entry_name == '.') { /* for now, ignore all files starting with '.' that haven't * already been handled (for future possibilities) */ - } else if(full) { + } else if(hit_mtree && config) { + /* we have all we need */ + break; + } else if(full && !hit_mtree) { + /* building the file list: expensive way */ if(add_entry_to_files_list(newpkg, &files_size, entry) < 0) { goto error; } -- 1.8.5.3

On Mon, Jan 27, 2014 at 5:02 PM, Florian Pritz wrote:

This is used to deduplicate code when using the mtree as the file list source.

Signed-off-by: Florian Pritz --- lib/libalpm/be_package.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index 149f281..066c220 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -353,6 +353,23 @@ int _alpm_pkg_validate_internal(alpm_handle_t *handle, }

/** + * Handle the existance of simple paths for _alpm_load_pkg_internal() + * @param pkg package to change + * @param path path to examine + * @return 0 if path doesn't match any rule, 1 if it has been handled + */ +static int handle_simple_path(alpm_pkg_t *pkg, const char *path) +{ + if(strcmp(path, ".INSTALL") == 0) { + pkg->scriptlet = 1; + } else { + return 0; + } + + return 1;

This felt a little weird to read and understand the flow. Why not just `return 1` from in the if block, drop the else completely, and return 0 at the end of the function? +}

+ +/** * Load a package and create the corresponding alpm_pkg_t struct. * @param handle the context handle * @param pkgfile path to the package file @@ -423,8 +440,8 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, } config = 1; continue; - } else if(strcmp(entry_name, ".INSTALL") == 0) { - newpkg->scriptlet = 1; + } else if(handle_simple_path(newpkg, entry_name)) { + continue; } else if(*entry_name == '.') { /* for now, ignore all files starting with '.' that haven't * already been handled (for future possibilities) */ -- 1.8.5.3

Signed-off-by: Florian Pritz --- v2: - Incorporated suggestions by Dave (take **data instead of *data) - Handle overflows RFC: systemd's greedy_realloc0() uses uint8_t instead of char for *newdata. Is that important or doesn't matter? lib/libalpm/util.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ lib/libalpm/util.h | 1 + 2 files changed, 47 insertions(+) diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 1bbc768..753cdd7 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -1287,6 +1287,52 @@ int _alpm_fnmatch(const void *pattern, const void *string) return fnmatch(pattern, string, 0); } +/** + * Think of this as realloc with error handling and + * automatic growing based on current/required + * + * @param data source memory space + * @param current size of the space pointed to be data + * @param required size you want + * @return new memory + */ +void *_alpm_realloc(void **data, size_t *current, const size_t required) +{ + size_t old_size = *current; + char *newdata; + size_t newsize = *current; + + if(*current >= required) { + return data; + } + + if(*current == 0) { + newsize = ALPM_BUFFER_SIZE; + } else if (required > (*current) * 2) { + newsize = required * 2; + } else { + newsize *= 2; + } + + /* check for overflows */ + if (newsize < required) { + return NULL; + } + + newdata = realloc(*data, newsize); + if(!newdata) { + _alpm_alloc_fail(newsize); + return NULL; + } + + /* ensure all new memory is zeroed out, in both the initial + * allocation and later reallocs */ + memset(newdata + old_size, 0, *current - old_size); + *current = newsize; + *data = newdata; + return newdata; +} + void _alpm_alloc_fail(size_t size) { fprintf(stderr, "alloc failure: could not allocate %zd bytes\n", size); diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h index b566868..031254a 100644 --- a/lib/libalpm/util.h +++ b/lib/libalpm/util.h @@ -141,6 +141,7 @@ int _alpm_raw_ncmp(const char *first, const char *second, size_t max); int _alpm_access(alpm_handle_t *handle, const char *dir, const char *file, int amode); int _alpm_fnmatch_patterns(alpm_list_t *patterns, const char *string); int _alpm_fnmatch(const void *pattern, const void *string); +void *_alpm_realloc(void **data, size_t *current, const size_t required); #ifndef HAVE_STRSEP char *strsep(char **, const char *); -- 1.8.5.3

Signed-off-by: Florian Pritz --- v2: Adjust for v2 of realloc patch. lib/libalpm/be_package.c | 57 +++++++++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 27 deletions(-) diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index 0801fc6..6ef84fc 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -369,6 +369,34 @@ static int handle_simple_path(alpm_pkg_t *pkg, const char *path) } /** + * Add a file to the files list for pkg. + * + * @param pkg package to add the file to + * @param files_size size of pkg->files.files + * @param entry archive entry of the file to add to the list + * @return <0 on error, 0 on success + */ +static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, struct archive_entry *entry) +{ + const size_t files_count = pkg->files.count; + alpm_file_t *current_file; + const char *path; + + if(!_alpm_realloc((void **)&pkg->files.files, files_size, (files_count + 1) * sizeof(alpm_file_t))) { + return -1; + } + + path = archive_entry_pathname(entry); + + current_file = pkg->files.files + files_count; + STRDUP(current_file->name, path, return -1); + current_file->size = archive_entry_size(entry); + current_file->mode = archive_entry_mode(entry); + pkg->files.count++; + return 0; +} + +/** * Load a package and create the corresponding alpm_pkg_t struct. * @param handle the context handle * @param pkgfile path to the package file @@ -445,34 +473,9 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, /* for now, ignore all files starting with '.' that haven't * already been handled (for future possibilities) */ } else if(full) { - const size_t files_count = newpkg->files.count; - alpm_file_t *current_file; - /* Keep track of all files for filelist generation */ - if(files_count >= files_size) { - size_t old_size = files_size; - alpm_file_t *newfiles; - if(files_size == 0) { - files_size = 4; - } else { - files_size *= 2; - } - newfiles = realloc(newpkg->files.files, - sizeof(alpm_file_t) * files_size); - if(!newfiles) { - _alpm_alloc_fail(sizeof(alpm_file_t) * files_size); - goto error; - } - /* ensure all new memory is zeroed out, in both the initial - * allocation and later reallocs */ - memset(newfiles + old_size, 0, - sizeof(alpm_file_t) * (files_size - old_size)); - newpkg->files.files = newfiles; + if(add_entry_to_files_list(newpkg, &files_size, entry) < 0) { + goto error; } - current_file = newpkg->files.files + files_count; - STRDUP(current_file->name, entry_name, goto error); - current_file->size = archive_entry_size(entry); - current_file->mode = archive_entry_mode(entry); - newpkg->files.count++; } if(archive_read_data_skip(archive)) { -- 1.8.5.3

On 01/28/14 at 11:12am, Florian Pritz wrote:

Signed-off-by: Florian Pritz --- v2: - Incorporated suggestions by Dave (take **data instead of *data) - Handle overflows

RFC: systemd's greedy_realloc0() uses uint8_t instead of char for *newdata. Is that important or doesn't matter?

lib/libalpm/util.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ lib/libalpm/util.h | 1 + 2 files changed, 47 insertions(+)

diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 1bbc768..753cdd7 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -1287,6 +1287,52 @@ int _alpm_fnmatch(const void *pattern, const void *string) return fnmatch(pattern, string, 0); }

+/** + * Think of this as realloc with error handling and + * automatic growing based on current/required + * + * @param data source memory space + * @param current size of the space pointed to be data + * @param required size you want + * @return new memory + */

This should mention the fact that this is grow-only and "greedy" so required is really the minimum needed and won't be the final size. A note about possibly handing back unused portions of the allocated memory might not hurt either.

+void *_alpm_realloc(void **data, size_t *current, const size_t required) +{ + size_t old_size = *current;

This variable is entirely unnecessary because you don't update current until you're about to return.

+ char *newdata; + size_t newsize = *current; + + if(*current >= required) { + return data; + } + + if(*current == 0) { + newsize = ALPM_BUFFER_SIZE; + } else if (required > (*current) * 2) { + newsize = required * 2; + } else { + newsize *= 2;

I think this will read better if you set newsize = current * 2 here instead of setting newsize = current above and doubling it.

+ } + + /* check for overflows */ + if (newsize < required) { + return NULL; + } + + newdata = realloc(*data, newsize); + if(!newdata) { + _alpm_alloc_fail(newsize); + return NULL; + } + + /* ensure all new memory is zeroed out, in both the initial + * allocation and later reallocs */ + memset(newdata + old_size, 0, *current - old_size);

This does nothing because *current still equals old_size.

+ *current = newsize; + *data = newdata; + return newdata; +} + void _alpm_alloc_fail(size_t size) { fprintf(stderr, "alloc failure: could not allocate %zd bytes\n", size); diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h index b566868..031254a 100644 --- a/lib/libalpm/util.h +++ b/lib/libalpm/util.h @@ -141,6 +141,7 @@ int _alpm_raw_ncmp(const char *first, const char *second, size_t max); int _alpm_access(alpm_handle_t *handle, const char *dir, const char *file, int amode); int _alpm_fnmatch_patterns(alpm_list_t *patterns, const char *string); int _alpm_fnmatch(const void *pattern, const void *string); +void *_alpm_realloc(void **data, size_t *current, const size_t required);

#ifndef HAVE_STRSEP char *strsep(char **, const char *); -- 1.8.5.3

On 01/28/14 at 11:14am, Florian Pritz wrote:

This greatly speeds up file list generation times by avoiding uncompressing the whole package.

pacman -S base with a deliberate file conflict: before: 9.1 seconds after: 2.2 seconds

Signed-off-by: Florian Pritz ---

v2: Adjust for v2 of realloc patch.

lib/libalpm/be_package.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 151 insertions(+), 2 deletions(-)

diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index 6ef84fc..d876967 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -380,14 +380,34 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, struct a { const size_t files_count = pkg->files.count; alpm_file_t *current_file; + mode_t type; + size_t pathlen; const char *path;

if(!_alpm_realloc((void **)&pkg->files.files, files_size, (files_count + 1) * sizeof(alpm_file_t))) { return -1; }

+ type = archive_entry_filetype(entry); path = archive_entry_pathname(entry);

+ pathlen = strlen(path); + + /* mtree paths don't contain a tailing slash, those we get from + * the archive directly do (expensive way) + * Other code relies on it to detect directories so add it here.*/ + if(type == AE_IFDIR && path[pathlen - 1] != '/') { + /* 2 = 1 for / + 1 for \0 */ + char *newpath = malloc(pathlen + 2); + if (!newpath) { + _alpm_alloc_fail(pathlen + 2); + return -1; + } + strcpy(newpath, path); + newpath[pathlen] = '/'; + newpath[pathlen + 1] = '\0'; + path = newpath;

Memory leak. Just assign to current_file->name directly and avoid the STRDUP for this case.

+ } current_file = pkg->files.files + files_count; STRDUP(current_file->name, path, return -1); current_file->size = archive_entry_size(entry); @@ -397,6 +417,122 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, struct a }

/** + * Generate a new file list from an mtree file and add it to the package. + * An existing file list will be free()d first. + * + * archive should point to an archive struct which is already at the + * position of the mtree's header. + * + * @param handle + * @param pkg package to add the file list to + * @param archive archive containing the mtree + * @return 0 on success, <0 on error + */ +static int build_filelist_from_mtree(alpm_handle_t *handle, alpm_pkg_t *pkg, struct archive *archive) +{ + int ret = 0;

We normally use 'ret' for or own return value.

+ size_t mtree_msize = 0; + size_t mtree_csize = 0;

These either need more descriptive names or a comment explaining what they are.

+ size_t files_size = 0; // we clean up the existing array so this is fine

We only use c-style comments

+ char *mtree_data = NULL; + struct archive *mtree; + struct archive_entry *mtree_entry = NULL; + + _alpm_log(handle, ALPM_LOG_DEBUG, "found mtree for package %s, getting file list\n", pkg->filename);

Please try to keep lines closer to 80 columns.

+ + /* throw away any files we might have already found */ + free(pkg->files.files);

You need to free the file names as well.

+ pkg->files.files = NULL; + pkg->files.count = 0; + + /* create a new archive to parse the mtree and load it from archive into memory */ + // TODO: split this into a function + if((mtree = archive_read_new()) == NULL) { + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + + _alpm_archive_read_support_filter_all(mtree); + archive_read_support_format_mtree(mtree); + + // TODO: split this into a function + while(1) { + ssize_t size; + + if(!_alpm_realloc((void **)&mtree_data, &mtree_msize, mtree_csize + ALPM_BUFFER_SIZE)) { + goto error; + } + + size = archive_read_data(archive, mtree_data + mtree_csize, ALPM_BUFFER_SIZE); + + if(size < 0) { + _alpm_log(handle, ALPM_LOG_ERROR, _("error while reading package %s: %s\n"), + pkg->filename, archive_error_string(archive)); + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + if(size == 0) { + break; + } + + mtree_csize += size; + } + + // TODO: need wrapper function for archive_read_open_memory for old libarchive versions? + if(archive_read_open_memory(mtree, mtree_data, mtree_csize)) { + _alpm_log(handle, ALPM_LOG_ERROR, _("error while reading mtree of package %s: %s\n"), + pkg->filename, archive_error_string(mtree)); + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + + while((ret = archive_read_next_header(mtree, &mtree_entry)) == ARCHIVE_OK) { + const char *path = archive_entry_pathname(mtree_entry); + char *p = NULL; + + /* strip leading "./" from path entries */ + if(path[0] == '.' && path[1] == '/') { + path += 2; + } + + if(handle_simple_path(pkg, path)) { + continue; + } + + if(*path == '.') { + continue; + }

Please add a comment why we're skipping all files that start with '.'. Maybe this should just be moved into handle_simple_path

+ + /* Apparently archive_entry_copy/set_pathname doesn't like to get it's own + * pointer and produces rather strange results */ + STRDUP(p, path, goto error); + archive_entry_copy_pathname(mtree_entry, p); + + if(add_entry_to_files_list(pkg, &files_size, mtree_entry) < 0) {

You've already extracted the entry's pathname everywhere you call add_entry_to_file_list, how about just passing the path as an additional parameter instead of dup'ing it and copying it back to the mtree_entry?

+ free(p); + goto error; + } + free(p); + } + + if(ret != ARCHIVE_EOF && ret != ARCHIVE_OK) { /* An error occurred */ + _alpm_log(handle, ALPM_LOG_ERROR, _("error while reading mtree of package %s: %s\n"), + pkg->filename, archive_error_string(mtree)); + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + + free(mtree_data); + _alpm_archive_read_free(mtree); + _alpm_log(handle, ALPM_LOG_DEBUG, "finished mtree reading for %s\n", pkg->filename); + return 0; +error: + free(mtree_data); + _alpm_archive_read_free(mtree); + return -1; +} + +/** * Load a package and create the corresponding alpm_pkg_t struct. * @param handle the context handle * @param pkgfile path to the package file @@ -406,7 +542,7 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, struct a alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, const char *pkgfile, int full) { - int ret, fd, config = 0; + int ret, fd, config, hit_mtree = 0; struct archive *archive; struct archive_entry *entry; alpm_pkg_t *newpkg; @@ -469,10 +605,23 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, continue; } else if(handle_simple_path(newpkg, entry_name)) { continue; + } else if(full && strcmp(entry_name, ".MTREE") == 0) { + /* building the file list: cheap way + * get the filelist from the mtree file rather than scanning + * the whole archive */ + if(build_filelist_from_mtree(handle, newpkg, archive) < 0) { + goto error; + } + hit_mtree = 1; + continue; } else if(*entry_name == '.') { /* for now, ignore all files starting with '.' that haven't * already been handled (for future possibilities) */ - } else if(full) { + } else if(hit_mtree && config) { + /* we have all we need */ + break;

This is kind of a strange place for this test and is similar to an existing check at the bottom of the loop. Could you move them to the top of the loop and consolidate them?

+ } else if(full && !hit_mtree) { + /* building the file list: expensive way */ if(add_entry_to_files_list(newpkg, &files_size, entry) < 0) { goto error; } -- 1.8.5.3

Signed-off-by: Florian Pritz --- v4: Pass path via argument (suggested by Andrew in patch 5 v2) lib/libalpm/be_package.c | 56 +++++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index 5980784..393f436 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -373,6 +373,33 @@ static int handle_simple_path(alpm_pkg_t *pkg, const char *path) } /** + * Add a file to the files list for pkg. + * + * @param pkg package to add the file to + * @param files_size size of pkg->files.files + * @param entry archive entry of the file to add to the list + * @param path path of the file to be added + * @return <0 on error, 0 on success + */ +static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, + struct archive_entry *entry, const char *path) +{ + const size_t files_count = pkg->files.count; + alpm_file_t *current_file; + + if(!_alpm_realloc((void **)&pkg->files.files, files_size, (files_count + 1) * sizeof(alpm_file_t))) { + return -1; + } + + current_file = pkg->files.files + files_count; + STRDUP(current_file->name, path, return -1); + current_file->size = archive_entry_size(entry); + current_file->mode = archive_entry_mode(entry); + pkg->files.count++; + return 0; +} + +/** * Load a package and create the corresponding alpm_pkg_t struct. * @param handle the context handle * @param pkgfile path to the package file @@ -446,34 +473,9 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, } else if(handle_simple_path(newpkg, entry_name)) { continue; } else if(full) { - const size_t files_count = newpkg->files.count; - alpm_file_t *current_file; - /* Keep track of all files for filelist generation */ - if(files_count >= files_size) { - size_t old_size = files_size; - alpm_file_t *newfiles; - if(files_size == 0) { - files_size = 4; - } else { - files_size *= 2; - } - newfiles = realloc(newpkg->files.files, - sizeof(alpm_file_t) * files_size); - if(!newfiles) { - _alpm_alloc_fail(sizeof(alpm_file_t) * files_size); - goto error; - } - /* ensure all new memory is zeroed out, in both the initial - * allocation and later reallocs */ - memset(newfiles + old_size, 0, - sizeof(alpm_file_t) * (files_size - old_size)); - newpkg->files.files = newfiles; + if(add_entry_to_files_list(newpkg, &files_size, entry, entry_name) < 0) { + goto error; } - current_file = newpkg->files.files + files_count; - STRDUP(current_file->name, entry_name, goto error); - current_file->size = archive_entry_size(entry); - current_file->mode = archive_entry_mode(entry); - newpkg->files.count++; } if(archive_read_data_skip(archive)) { -- 1.8.5.3

On 31/01/14 02:24, Florian Pritz wrote:

Signed-off-by: Florian Pritz ---

v3: Implement suggestions from Andrew

RFC from v2 still holds: is using char for *newdata fine or is there any good reason to use uint8_t like systemd does?

lib/libalpm/util.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ lib/libalpm/util.h | 1 + 2 files changed, 50 insertions(+)

diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 150b85e..28b8646 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -1287,6 +1287,55 @@ int _alpm_fnmatch(const void *pattern, const void *string) return fnmatch(pattern, string, 0); }

+/** + * Think of this as realloc with error handling and + * automatic growing based on current/required. + * + * data will point to a memory space with at least required + * bytes, likely more. You may want to free unused memory. + * If required < current data is returned and nothing happens. + * + * @param data source memory space + * @param current size of the space pointed to be data + * @param required size you want + * @return new memory if grown; old memory otherwise; NULL on error + */ +void *_alpm_realloc(void **data, size_t *current, const size_t required) +{ + char *newdata; + size_t newsize = 0; + + if(*current >= required) { + return data; + } + + if(*current == 0) { + newsize = ALPM_BUFFER_SIZE; + } else if (required > (*current) * 2) { + newsize = required * 2; + } else { + newsize = *current * 2; + }

What is this? It takes a "required" parameter and then returns whatever it feels like and ...

+ /* check for overflows */ + if (newsize < required) { + return NULL; + }

... doing that can cause an overflow. I'd say the correct place for an overflow check should be when calculating required before calling this function.

+ newdata = realloc(*data, newsize); + if(!newdata) { + _alpm_alloc_fail(newsize); + return NULL; + } + + /* ensure all new memory is zeroed out, in both the initial + * allocation and later reallocs */ + memset(newdata + *current, 0, newsize - *current);

Do we really need that memset? Anyway, I see no need for this function at all. Just use plain realloc and check the return. You still need to check what this returns anyway. It is more work for zero gain, particularly if you just use ALPM_BUFFER_SIZE at the start and double the size each realloc. It is exactly what your formula currently does because "required > (*current) * 2" can never happen in your filelist reading from mtree function in patch #5. As an aside, using packages with mtree files on my system: 47.7% of packages will not need a realloc 15.5% will need 1 realloc 14.3% will need 2 8.6% will need 3 6.6% will need 4 ... I found 1 package on my system needing 8. That looks fine (maybe decays a bit slow, but it will do). Allan

On 02/02/14 04:12, Florian Pritz wrote:

This greatly speeds up file list generation times by avoiding uncompressing the whole package.

pacman -S base with a deliberate file conflict: before: 9.1 seconds after: 2.2 seconds

Signed-off-by: Florian Pritz ---

v4:

- fix mem leak when cleaning up pkg->files - make mtree_*size names a little more descriptive - switch comments to c style - break some long lines, there are still a few left.. - files starting with . are handled by handle_simple_path now - pass the path directly to add_entry_to_files_list - consolidate early breaking in main loop

NOT changed:

- ret for archive_read_next_header: copied from original code in _alpm_pkg_load_internal, if anyone cares feel free to change it later - I didn't break all lines > 80 chars, that should probably be done in a line break cleanup only commit

lib/libalpm/be_package.c | 150 +++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 146 insertions(+), 4 deletions(-)

diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index 393f436..ac52ae1 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -386,13 +386,36 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, { const size_t files_count = pkg->files.count; alpm_file_t *current_file; + mode_t type; + size_t pathlen;

if(!_alpm_realloc((void **)&pkg->files.files, files_size, (files_count + 1) * sizeof(alpm_file_t))) { return -1; }

+ type = archive_entry_filetype(entry); + + pathlen = strlen(path); + current_file = pkg->files.files + files_count; - STRDUP(current_file->name, path, return -1); + + /* mtree paths don't contain a tailing slash, those we get from + * the archive directly do (expensive way) + * Other code relies on it to detect directories so add it here.*/ + if(type == AE_IFDIR && path[pathlen - 1] != '/') { + /* 2 = 1 for / + 1 for \0 */ + char *newpath = malloc(pathlen + 2); + if (!newpath) { + _alpm_alloc_fail(pathlen + 2); + return -1; + } + strcpy(newpath, path); + newpath[pathlen] = '/'; + newpath[pathlen + 1] = '\0'; + current_file->name = newpath; + } else { + STRDUP(current_file->name, path, return -1); + } current_file->size = archive_entry_size(entry); current_file->mode = archive_entry_mode(entry); pkg->files.count++; @@ -400,6 +423,115 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, }

/** + * Generate a new file list from an mtree file and add it to the package. + * An existing file list will be free()d first. + * + * archive should point to an archive struct which is already at the + * position of the mtree's header. + * + * @param handle + * @param pkg package to add the file list to + * @param archive archive containing the mtree + * @return 0 on success, <0 on error + */ +static int build_filelist_from_mtree(alpm_handle_t *handle, alpm_pkg_t *pkg, struct archive *archive) +{ + int ret = 0; + size_t mtree_maxsize = 0; + size_t mtree_cursize = 0; + size_t files_size = 0; /* we clean up the existing array so this is fine */ + char *mtree_data = NULL; + struct archive *mtree; + struct archive_entry *mtree_entry = NULL; + + _alpm_log(handle, ALPM_LOG_DEBUG, + "found mtree for package %s, getting file list\n", pkg->filename); + + /* throw away any files we might have already found */ + for (size_t i = 0; i < pkg->files.count; i++) { + free(pkg->files.files[i].name); + } + free(pkg->files.files); + pkg->files.files = NULL; + pkg->files.count = 0; + + /* create a new archive to parse the mtree and load it from archive into memory */ + /* TODO: split this into a function */ + if((mtree = archive_read_new()) == NULL) { + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + + _alpm_archive_read_support_filter_all(mtree); + archive_read_support_format_mtree(mtree); + + /* TODO: split this into a function */ + while(1) { + ssize_t size; + + if(!_alpm_realloc((void **)&mtree_data, &mtree_maxsize, mtree_cursize + ALPM_BUFFER_SIZE)) { + goto error; + } + + size = archive_read_data(archive, mtree_data + mtree_cursize, ALPM_BUFFER_SIZE); + + if(size < 0) { + _alpm_log(handle, ALPM_LOG_ERROR, _("error while reading package %s: %s\n"), + pkg->filename, archive_error_string(archive)); + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + if(size == 0) { + break; + } + + mtree_cursize += size; + } + + /* TODO: need wrapper function for archive_read_open_memory for old libarchive versions? */

When did support for that function get added?

+ if(archive_read_open_memory(mtree, mtree_data, mtree_cursize)) { + _alpm_log(handle, ALPM_LOG_ERROR, + _("error while reading mtree of package %s: %s\n"), + pkg->filename, archive_error_string(mtree)); + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + + while((ret = archive_read_next_header(mtree, &mtree_entry)) == ARCHIVE_OK) { + const char *path = archive_entry_pathname(mtree_entry); + + /* strip leading "./" from path entries */ + if(path[0] == '.' && path[1] == '/') { + path += 2; + } + + if(handle_simple_path(pkg, path)) { + continue; + } + + if(add_entry_to_files_list(pkg, &files_size, mtree_entry, path) < 0) { + goto error; + } + } + + if(ret != ARCHIVE_EOF && ret != ARCHIVE_OK) { /* An error occurred */ + _alpm_log(handle, ALPM_LOG_ERROR, _("error while reading mtree of package %s: %s\n"), + pkg->filename, archive_error_string(mtree)); + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + + free(mtree_data); + _alpm_archive_read_free(mtree); + _alpm_log(handle, ALPM_LOG_DEBUG, "finished mtree reading for %s\n", pkg->filename); + return 0; +error: + free(mtree_data); + _alpm_archive_read_free(mtree); + return -1; +} + +/** * Load a package and create the corresponding alpm_pkg_t struct. * @param handle the context handle * @param pkgfile path to the package file @@ -409,7 +541,7 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, const char *pkgfile, int full) { - int ret, fd, config = 0; + int ret, fd, config, hit_mtree = 0; struct archive *archive; struct archive_entry *entry; alpm_pkg_t *newpkg; @@ -472,7 +604,17 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, continue; } else if(handle_simple_path(newpkg, entry_name)) { continue;

handle_simple_path will return 1 if the file starts with a ".". ".MTREE" starts with a ".", so....

- } else if(full) { + } else if(full && strcmp(entry_name, ".MTREE") == 0) {

... this can never happen.

+ /* building the file list: cheap way + * get the filelist from the mtree file rather than scanning + * the whole archive */ + if(build_filelist_from_mtree(handle, newpkg, archive) < 0) { + goto error; + } + hit_mtree = 1; + continue; + } else if(full && !hit_mtree) { + /* building the file list: expensive way */ if(add_entry_to_files_list(newpkg, &files_size, entry, entry_name) < 0) { goto error; } @@ -486,7 +628,7 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, }

/* if we are not doing a full read, see if we have all we need */ - if(!full && config) { + if((!full || hit_mtree) && config) {

Why does hit_mtree matter here? if(!full) we should never read the mtree file.

break; } }

On 03/02/14 01:32, Florian Pritz wrote:

On 02.02.2014 12:13, Allan McRae wrote:

...

On 02/02/14 04:12, Florian Pritz wrote:

...

This greatly speeds up file list generation times by avoiding uncompressing the whole package.

pacman -S base with a deliberate file conflict: before: 9.1 seconds after: 2.2 seconds

Signed-off-by: Florian Pritz ---

v4:

- fix mem leak when cleaning up pkg->files - make mtree_*size names a little more descriptive - switch comments to c style - break some long lines, there are still a few left.. - files starting with . are handled by handle_simple_path now - pass the path directly to add_entry_to_files_list - consolidate early breaking in main loop

NOT changed:

- ret for archive_read_next_header: copied from original code in _alpm_pkg_load_internal, if anyone cares feel free to change it later - I didn't break all lines > 80 chars, that should probably be done in a line break cleanup only commit

lib/libalpm/be_package.c | 150 +++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 146 insertions(+), 4 deletions(-)

diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index 393f436..ac52ae1 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -386,13 +386,36 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, { const size_t files_count = pkg->files.count; alpm_file_t *current_file; + mode_t type; + size_t pathlen;

if(!_alpm_realloc((void **)&pkg->files.files, files_size, (files_count + 1) * sizeof(alpm_file_t))) { return -1; }

+ type = archive_entry_filetype(entry); + + pathlen = strlen(path); + current_file = pkg->files.files + files_count; - STRDUP(current_file->name, path, return -1); + + /* mtree paths don't contain a tailing slash, those we get from + * the archive directly do (expensive way) + * Other code relies on it to detect directories so add it here.*/ + if(type == AE_IFDIR && path[pathlen - 1] != '/') { + /* 2 = 1 for / + 1 for \0 */ + char *newpath = malloc(pathlen + 2); + if (!newpath) { + _alpm_alloc_fail(pathlen + 2); + return -1; + } + strcpy(newpath, path); + newpath[pathlen] = '/'; + newpath[pathlen + 1] = '\0'; + current_file->name = newpath; + } else { + STRDUP(current_file->name, path, return -1); + } current_file->size = archive_entry_size(entry); current_file->mode = archive_entry_mode(entry); pkg->files.count++; @@ -400,6 +423,115 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, }

/** + * Generate a new file list from an mtree file and add it to the package. + * An existing file list will be free()d first. + * + * archive should point to an archive struct which is already at the + * position of the mtree's header. + * + * @param handle + * @param pkg package to add the file list to + * @param archive archive containing the mtree + * @return 0 on success, <0 on error + */ +static int build_filelist_from_mtree(alpm_handle_t *handle, alpm_pkg_t *pkg, struct archive *archive) +{ + int ret = 0; + size_t mtree_maxsize = 0; + size_t mtree_cursize = 0; + size_t files_size = 0; /* we clean up the existing array so this is fine */ + char *mtree_data = NULL; + struct archive *mtree; + struct archive_entry *mtree_entry = NULL; + + _alpm_log(handle, ALPM_LOG_DEBUG, + "found mtree for package %s, getting file list\n", pkg->filename); + + /* throw away any files we might have already found */ + for (size_t i = 0; i < pkg->files.count; i++) { + free(pkg->files.files[i].name); + } + free(pkg->files.files); + pkg->files.files = NULL; + pkg->files.count = 0; + + /* create a new archive to parse the mtree and load it from archive into memory */ + /* TODO: split this into a function */ + if((mtree = archive_read_new()) == NULL) { + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + + _alpm_archive_read_support_filter_all(mtree); + archive_read_support_format_mtree(mtree); + + /* TODO: split this into a function */ + while(1) { + ssize_t size; + + if(!_alpm_realloc((void **)&mtree_data, &mtree_maxsize, mtree_cursize + ALPM_BUFFER_SIZE)) { + goto error; + } + + size = archive_read_data(archive, mtree_data + mtree_cursize, ALPM_BUFFER_SIZE); + + if(size < 0) { + _alpm_log(handle, ALPM_LOG_ERROR, _("error while reading package %s: %s\n"), + pkg->filename, archive_error_string(archive)); + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + if(size == 0) { + break; + } + + mtree_cursize += size; + } + + /* TODO: need wrapper function for archive_read_open_memory for old libarchive versions? */

When did support for that function get added?

Seems like it's been there from the beginning of the git repo (2007) so I guess I can remove that comment.

...

...

+ if(archive_read_open_memory(mtree, mtree_data, mtree_cursize)) { + _alpm_log(handle, ALPM_LOG_ERROR, + _("error while reading mtree of package %s: %s\n"), + pkg->filename, archive_error_string(mtree)); + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + + while((ret = archive_read_next_header(mtree, &mtree_entry)) == ARCHIVE_OK) { + const char *path = archive_entry_pathname(mtree_entry); + + /* strip leading "./" from path entries */ + if(path[0] == '.' && path[1] == '/') { + path += 2; + } + + if(handle_simple_path(pkg, path)) { + continue; + } + + if(add_entry_to_files_list(pkg, &files_size, mtree_entry, path) < 0) { + goto error; + } + } + + if(ret != ARCHIVE_EOF && ret != ARCHIVE_OK) { /* An error occurred */ + _alpm_log(handle, ALPM_LOG_ERROR, _("error while reading mtree of package %s: %s\n"), + pkg->filename, archive_error_string(mtree)); + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + + free(mtree_data); + _alpm_archive_read_free(mtree); + _alpm_log(handle, ALPM_LOG_DEBUG, "finished mtree reading for %s\n", pkg->filename); + return 0; +error: + free(mtree_data); + _alpm_archive_read_free(mtree); + return -1; +} + +/** * Load a package and create the corresponding alpm_pkg_t struct. * @param handle the context handle * @param pkgfile path to the package file @@ -409,7 +541,7 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, const char *pkgfile, int full) { - int ret, fd, config = 0; + int ret, fd, config, hit_mtree = 0; struct archive *archive; struct archive_entry *entry; alpm_pkg_t *newpkg; @@ -472,7 +604,17 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, continue; } else if(handle_simple_path(newpkg, entry_name)) { continue;

handle_simple_path will return 1 if the file starts with a ".". ".MTREE" starts with a ".", so....

...

- } else if(full) { + } else if(full && strcmp(entry_name, ".MTREE") == 0) {

... this can never happen.

Thanks, messed that up when rebasing some other changes.

...

...

+ /* building the file list: cheap way + * get the filelist from the mtree file rather than scanning + * the whole archive */ + if(build_filelist_from_mtree(handle, newpkg, archive) < 0) { + goto error; + } + hit_mtree = 1; + continue; + } else if(full && !hit_mtree) { + /* building the file list: expensive way */ if(add_entry_to_files_list(newpkg, &files_size, entry, entry_name) < 0) { goto error; } @@ -486,7 +628,7 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, }

/* if we are not doing a full read, see if we have all we need */ - if(!full && config) { + if((!full || hit_mtree) && config) {

Why does hit_mtree matter here? if(!full) we should never read the mtree file.

hit_mtree matters because we want to break the loop once we have a file list (if we need one), but only if we also have read .PKGINFO.

Since .MTREE could be before .PKGINFO in the tarball I can't break just after reading the mtree.

Does that make sense?

Sure - I was making assumptions given the order of the files in the tarball. Allan

Signed-off-by: Florian Pritz --- lib/libalpm/add.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index f38afef..1199d30 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -178,6 +178,11 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, archive_read_data_skip(archive); return 0; } else { + if (!alpm_filelist_contains(&newpkg->files, entryname)) { + _alpm_log(handle, ALPM_LOG_WARNING, _("file not found in file list for package %s. skipping extraction of %s\n"), + newpkg->name, entryname); + return 0; + } /* build the new entryname relative to handle->root */ snprintf(filename, PATH_MAX, "%s%s", handle->root, entryname); } -- 1.8.5.3

This is used to deduplicate code when using the mtree as the file list source. Signed-off-by: Florian Pritz --- lib/libalpm/be_package.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index 2fbb1db..5980784 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -353,6 +353,26 @@ int _alpm_pkg_validate_internal(alpm_handle_t *handle, } /** + * Handle the existance of simple paths for _alpm_load_pkg_internal() + * @param pkg package to change + * @param path path to examine + * @return 0 if path doesn't match any rule, 1 if it has been handled + */ +static int handle_simple_path(alpm_pkg_t *pkg, const char *path) +{ + if(strcmp(path, ".INSTALL") == 0) { + pkg->scriptlet = 1; + return 1; + } else if(*path == '.') { + /* for now, ignore all files starting with '.' that haven't + * already been handled (for future possibilities) */ + return 1; + } + + return 0; +} + +/** * Load a package and create the corresponding alpm_pkg_t struct. * @param handle the context handle * @param pkgfile path to the package file @@ -423,11 +443,8 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, } config = 1; continue; - } else if(strcmp(entry_name, ".INSTALL") == 0) { - newpkg->scriptlet = 1; - } else if(*entry_name == '.') { - /* for now, ignore all files starting with '.' that haven't - * already been handled (for future possibilities) */ + } else if(handle_simple_path(newpkg, entry_name)) { + continue; } else if(full) { const size_t files_count = newpkg->files.count; alpm_file_t *current_file; -- 1.8.5.3

This greatly speeds up file list generation times by avoiding uncompressing the whole package. pacman -S base with a deliberate file conflict: before: 9.1 seconds after: 2.2 seconds Signed-off-by: Florian Pritz --- lib/libalpm/be_package.c | 149 +++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 145 insertions(+), 4 deletions(-) diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index e276f53..3c35484 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -386,13 +386,36 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, { const size_t files_count = pkg->files.count; alpm_file_t *current_file; + mode_t type; + size_t pathlen; if(!_alpm_greedy_grow((void **)&pkg->files.files, files_size, (files_count + 1) * sizeof(alpm_file_t))) { return -1; } + type = archive_entry_filetype(entry); + + pathlen = strlen(path); + current_file = pkg->files.files + files_count; - STRDUP(current_file->name, path, return -1); + + /* mtree paths don't contain a tailing slash, those we get from + * the archive directly do (expensive way) + * Other code relies on it to detect directories so add it here.*/ + if(type == AE_IFDIR && path[pathlen - 1] != '/') { + /* 2 = 1 for / + 1 for \0 */ + char *newpath = malloc(pathlen + 2); + if (!newpath) { + _alpm_alloc_fail(pathlen + 2); + return -1; + } + strcpy(newpath, path); + newpath[pathlen] = '/'; + newpath[pathlen + 1] = '\0'; + current_file->name = newpath; + } else { + STRDUP(current_file->name, path, return -1); + } current_file->size = archive_entry_size(entry); current_file->mode = archive_entry_mode(entry); pkg->files.count++; @@ -400,6 +423,114 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, } /** + * Generate a new file list from an mtree file and add it to the package. + * An existing file list will be free()d first. + * + * archive should point to an archive struct which is already at the + * position of the mtree's header. + * + * @param handle + * @param pkg package to add the file list to + * @param archive archive containing the mtree + * @return 0 on success, <0 on error + */ +static int build_filelist_from_mtree(alpm_handle_t *handle, alpm_pkg_t *pkg, struct archive *archive) +{ + int ret = 0; + size_t mtree_maxsize = 0; + size_t mtree_cursize = 0; + size_t files_size = 0; /* we clean up the existing array so this is fine */ + char *mtree_data = NULL; + struct archive *mtree; + struct archive_entry *mtree_entry = NULL; + + _alpm_log(handle, ALPM_LOG_DEBUG, + "found mtree for package %s, getting file list\n", pkg->filename); + + /* throw away any files we might have already found */ + for (size_t i = 0; i < pkg->files.count; i++) { + free(pkg->files.files[i].name); + } + free(pkg->files.files); + pkg->files.files = NULL; + pkg->files.count = 0; + + /* create a new archive to parse the mtree and load it from archive into memory */ + /* TODO: split this into a function */ + if((mtree = archive_read_new()) == NULL) { + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + + _alpm_archive_read_support_filter_all(mtree); + archive_read_support_format_mtree(mtree); + + /* TODO: split this into a function */ + while(1) { + ssize_t size; + + if(!_alpm_greedy_grow((void **)&mtree_data, &mtree_maxsize, mtree_cursize + ALPM_BUFFER_SIZE)) { + goto error; + } + + size = archive_read_data(archive, mtree_data + mtree_cursize, ALPM_BUFFER_SIZE); + + if(size < 0) { + _alpm_log(handle, ALPM_LOG_ERROR, _("error while reading package %s: %s\n"), + pkg->filename, archive_error_string(archive)); + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + if(size == 0) { + break; + } + + mtree_cursize += size; + } + + if(archive_read_open_memory(mtree, mtree_data, mtree_cursize)) { + _alpm_log(handle, ALPM_LOG_ERROR, + _("error while reading mtree of package %s: %s\n"), + pkg->filename, archive_error_string(mtree)); + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + + while((ret = archive_read_next_header(mtree, &mtree_entry)) == ARCHIVE_OK) { + const char *path = archive_entry_pathname(mtree_entry); + + /* strip leading "./" from path entries */ + if(path[0] == '.' && path[1] == '/') { + path += 2; + } + + if(handle_simple_path(pkg, path)) { + continue; + } + + if(add_entry_to_files_list(pkg, &files_size, mtree_entry, path) < 0) { + goto error; + } + } + + if(ret != ARCHIVE_EOF && ret != ARCHIVE_OK) { /* An error occurred */ + _alpm_log(handle, ALPM_LOG_ERROR, _("error while reading mtree of package %s: %s\n"), + pkg->filename, archive_error_string(mtree)); + handle->pm_errno = ALPM_ERR_LIBARCHIVE; + goto error; + } + + free(mtree_data); + _alpm_archive_read_free(mtree); + _alpm_log(handle, ALPM_LOG_DEBUG, "finished mtree reading for %s\n", pkg->filename); + return 0; +error: + free(mtree_data); + _alpm_archive_read_free(mtree); + return -1; +} + +/** * Load a package and create the corresponding alpm_pkg_t struct. * @param handle the context handle * @param pkgfile path to the package file @@ -409,7 +540,7 @@ static int add_entry_to_files_list(alpm_pkg_t *pkg, size_t *files_size, alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, const char *pkgfile, int full) { - int ret, fd, config = 0; + int ret, fd, config, hit_mtree = 0; struct archive *archive; struct archive_entry *entry; alpm_pkg_t *newpkg; @@ -470,9 +601,19 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, } config = 1; continue; + } else if(full && strcmp(entry_name, ".MTREE") == 0) { + /* building the file list: cheap way + * get the filelist from the mtree file rather than scanning + * the whole archive */ + if(build_filelist_from_mtree(handle, newpkg, archive) < 0) { + goto error; + } + hit_mtree = 1; + continue; } else if(handle_simple_path(newpkg, entry_name)) { continue; - } else if(full) { + } else if(full && !hit_mtree) { + /* building the file list: expensive way */ if(add_entry_to_files_list(newpkg, &files_size, entry, entry_name) < 0) { goto error; } @@ -486,7 +627,7 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, } /* if we are not doing a full read, see if we have all we need */ - if(!full && config) { + if((!full || hit_mtree) && config) { break; } } -- 1.8.5.3

On 10/02/14 05:24, Florian Pritz wrote:

Signed-off-by: Florian Pritz --- lib/libalpm/be_local.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/lib/libalpm/be_local.c b/lib/libalpm/be_local.c index 0b5b266..d2b0fe3 100644 --- a/lib/libalpm/be_local.c +++ b/lib/libalpm/be_local.c @@ -736,22 +736,8 @@ static int local_db_read(alpm_pkg_t *info, alpm_dbinfrq_t inforeq)

while(fgets(line, sizeof(line), fp) && (len = _alpm_strip_newline(line, 0))) { - if(files_count >= files_size) { - size_t old_size = files_size; - if(files_size == 0) { - files_size = 8; - } else { - files_size *= 2; - } - files = realloc(files, sizeof(alpm_file_t) * files_size); - if(!files) { - _alpm_alloc_fail(sizeof(alpm_file_t) * files_size); - goto error; - } - /* ensure all new memory is zeroed out, in both the initial - * allocation and later reallocs */ - memset(files + old_size, 0, - sizeof(alpm_file_t) * (files_size - old_size)); + if(!_alpm_greedy_grow((void **)&files, &files_size, (files_count + 1) * sizeof(alpm_file_t))) {

This starts the size at 1 then doubles rather than starting at 8 like previously. This does the old way: _alpm_greedy_grow((void **)&files, &files_size, (files_size ? files_size + 1 : 8 * sizeof(alpm_file_t)))

+ goto error; } /* since we know the length of the file string already, * we can do malloc + memcpy rather than strdup */

Signed-off-by: Florian Pritz --- v6: Allocate 8 elements at the beginning instead of simply one. lib/libalpm/be_local.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/lib/libalpm/be_local.c b/lib/libalpm/be_local.c index 0b5b266..0a86f11 100644 --- a/lib/libalpm/be_local.c +++ b/lib/libalpm/be_local.c @@ -736,22 +736,10 @@ static int local_db_read(alpm_pkg_t *info, alpm_dbinfrq_t inforeq) while(fgets(line, sizeof(line), fp) && (len = _alpm_strip_newline(line, 0))) { - if(files_count >= files_size) { - size_t old_size = files_size; - if(files_size == 0) { - files_size = 8; - } else { - files_size *= 2; - } - files = realloc(files, sizeof(alpm_file_t) * files_size); - if(!files) { - _alpm_alloc_fail(sizeof(alpm_file_t) * files_size); - goto error; - } - /* ensure all new memory is zeroed out, in both the initial - * allocation and later reallocs */ - memset(files + old_size, 0, - sizeof(alpm_file_t) * (files_size - old_size)); + _alpm_log(db->handle, ALPM_LOG_DEBUG, "allocating %ld\n", (files_size ? files_size + 1 : 8 * sizeof(alpm_file_t))); + if(!_alpm_greedy_grow((void **)&files, &files_size, + (files_size ? files_size + sizeof(alpm_file_t) : 8 * sizeof(alpm_file_t)))) { + goto error; } /* since we know the length of the file string already, * we can do malloc + memcpy rather than strdup */ -- 1.8.5.3