Re: [pacman-dev] [PATCH v3 2/5] util: Add _alpm_realloc()
On 31/01/14 02:24, Florian Pritz wrote:
Signed-off-by: Florian Pritz bluew...@xinu.at
---
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
Re: [pacman-dev] [PATCH v3 2/5] util: Add _alpm_realloc()
On 02/02/14 20:39, Allan McRae wrote:
On 31/01/14 02:24, Florian Pritz wrote:
Signed-off-by: Florian Pritz bluew...@xinu.at
---
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.
Reading through the rest of your patches and seeing you are refactoring
some of the other realloc calls into this, I'd be happy if this was
just a wrapper to realloc that did the memset. I.e. You just realloc
to the required value. Then do the memset if current required.
Allan
Re: [pacman-dev] [PATCH] Put not explicitly installed packages in greyduring update/install/removall
Of the two options, I would prefer the dependencies to be shown in grey instead of having explicitly installed packages typeset in bold. I don't feel too strongly about this though, so I won't complain if everyone else prefers bold. There is a second option. Instead of just flaging deps OR explicit and associating a color with both, we can flag both and associate a color with both, and let this color be changed by configuration (i.e., the line in conf.c/enable_color) so everybody will be happy... -- G.
Re: [pacman-dev] [PATCH v4 5/5] be_package: Build the file list from MTREE if possible
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 bluew...@xinu.at
---
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;
+
Re: [pacman-dev] [PATCH] Put not explicitly installed packages in grey during update/install/removall
On 02/02/14 07:41, Guillaume Bouchard wrote:
In extended table view, packages which are not explicitly installed are
displayed in grey. This helps understanding why
packages are updated.
This helps the package managment workflow. During update, we can
quickly have a look at why packages are updated and easilly track and
remove the explicit packages which are not longer required. During
remove, it shows all the explicit packages which are also removed by a
cascade removal. During install, it provides a feedback on how your
action will affect the database.
This patch created a new colstr setting and associate it with BOLDBLACK
(i.e. grey).
That colour cases the dependencies to stand out more on a terminal with
a white background. I'd say bold would be better...
However, the colour coding really is unclear. How do people come into
the knowledge of what it means? For example, during an update I might
think that a new package being pulled in as a dependency so it is
highlighted. Or is it entirely obvious and I am thinking too hard?
Allan
Signed-off-by: Guillaume Bouchard guillaum.bouch...@gmail.com
---
src/pacman/conf.c | 1 +
src/pacman/conf.h | 1 +
src/pacman/util.c | 13 ++---
3 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c
index 4b7ec05..243203f 100644
--- a/src/pacman/conf.c
+++ b/src/pacman/conf.c
@@ -76,6 +76,7 @@ void enable_colors(int colors)
colstr-warn= BOLDYELLOW;
colstr-err = BOLDRED;
colstr-nocolor = NOCOLOR;
+ colstr-depend = BOLDBLACK;
}
}
diff --git a/src/pacman/conf.h b/src/pacman/conf.h
index e8cac50..4af5eeb 100644
--- a/src/pacman/conf.h
+++ b/src/pacman/conf.h
@@ -32,6 +32,7 @@ typedef struct __colstr_t {
const char *warn;
const char *err;
const char *nocolor;
+ const char *depend;
} colstr_t;
typedef struct __config_t {
diff --git a/src/pacman/util.c b/src/pacman/util.c
index cbe371d..699d347 100644
--- a/src/pacman/util.c
+++ b/src/pacman/util.c
@@ -57,7 +57,8 @@ enum {
CELL_NORMAL = 0,
CELL_TITLE = (1 0),
CELL_RIGHT_ALIGN = (1 1),
- CELL_FREE = (1 3)
+ CELL_FREE = (1 3),
+ CELL_DEPEND = (1 4)
};
int trans_init(alpm_transflag_t flags, int check_valid)
@@ -488,7 +489,9 @@ static void table_print_line(const alpm_list_t *line,
short col_padding,
if(cell-mode CELL_TITLE) {
printf(%s%*s%s, config-colstr.title, cell_width,
str, config-colstr.nocolor);
- } else {
+ } else if(cell-mode CELL_DEPEND) {
+ printf(%s%*s%s, config-colstr.depend, cell_width,
str, config-colstr.nocolor);
+ } else {
printf(%*s, cell_width, str);
}
need_padding = 1;
@@ -808,7 +811,11 @@ static alpm_list_t *create_verbose_row(pm_target_t
*target)
} else {
pm_asprintf(str, %s, alpm_pkg_get_name(target-remove));
}
- add_table_cell(ret, str, CELL_NORMAL | CELL_FREE);
+ if(alpm_pkg_get_reason(target-remove ? target-remove :
target-install) == ALPM_PKG_REASON_DEPEND) {
+ add_table_cell(ret, str, CELL_DEPEND | CELL_FREE);
+ } else {
+ add_table_cell(ret, str, CELL_NORMAL | CELL_FREE);
+ }
/* old and new versions */
pm_asprintf(str, %s,
Re: [pacman-dev] [PATCH] checkupdates: Add package download mode with -d
On 17/01/14 15:44, Chris Down wrote:
This new mode, which must be run as root, allows for the precaching of
packages which can then subsequently be updated by calling
pacman -Syu as normal. This can then be integrated with a cron script
(for example, checkupdates-cron[0]) for automated, safe downloading of
packages alongside getting the list of packages to update, or run
manually on the command line to download new packages, but avoid
updating the main package database.
As a side effect of adding the -d option, argument handling (which was
previously not present) is now done using getopts. This seems like a
reasonable way to do it instead of just hacking in a check for -d as
$1 now, but I'm not particularly averse to either. getopts would just
allow for better future flexibility in this area.
It is possible to do this as another script, say, downloadupdates, but
we would encounter pretty heavy code duplication with checkupdates, so
it seems more sensible to do it here.
Due to the use of --noconfirm, this has some limitations when we
encounter more complex upgrade scenarios, for example when a new package
conflict occurs, an upgrade path cannot be correctly/automatically
determined, a package was replaced, etc. This is not a major problem --
in the worst case, we just don't get the packages that we wanted
downloaded and an error message is displayed to the user.
The choice to hide both STDOUT and STDERR of pacman -Syuw is
potentially up for debate. It makes it more difficult to determine where
the problem was if the command fails -- we still display a message
saying that that was the case, and set the exit code accordingly, but
the actual cause needs to be determined by the user elsewhere. For this
reason, -p exists to force the package download to output as normal.
It seems desirable that -p also affects the silence of the database
update, but this is also up for discussion if people feel it should not
be within the remit of the (( silent )) checks.
[0]: https://github.com/cdown/checkupdates-cron
So there are two completely separate changes here...Adding -d and
-p. I'd say -p should be -v or --verbose. These patches
need separately submitted.
I really do not think getopt is justified here. There is nothing
complex. Just loop through the arguments.
Also, the output of checkupdates is simple for including in a script. I
think checkupdates should just stay checking updates.
export CHECKUPDATES_DB=/some/db/path
updates=($(checkupdates))
if [[ -n ${updates[@]} ]]; then
pacman -Sw --dbpath $CHECKUPDATES_DB $updates
done
---
contrib/checkupdates.sh.in | 41 ++---
1 file changed, 38 insertions(+), 3 deletions(-)
diff --git a/contrib/checkupdates.sh.in b/contrib/checkupdates.sh.in
index 413b78a..02acb67 100644
--- a/contrib/checkupdates.sh.in
+++ b/contrib/checkupdates.sh.in
@@ -21,16 +21,43 @@
declare -r myname='checkupdates'
declare -r myver='@PACKAGE_VERSION@'
-if (( $# 0 )); then
+download=0
+silent=1
+
+usage() {
echo ${myname} (pacman) v${myver}
echo
echo Safely print a list of pending updates
echo
echo Usage: ${myname}
echo
+ echo Options:
+ echo -d Download upgradeable packages to pacman package cache.
+ echo -p Print pacman output from the database upgrade and package
download.
+ echo
echo 'Note: Export the CHECKUPDATES_DB variable to change the path of
the temporary database.'
exit 0
-fi
+}
+
+run_silent() (
+ (( silent )) exec /dev/null
+ $@
+)
+
+while getopts ':dp' opt; do
+ case $opt in
+ d)
+ if (( UID )); then
+ echo Download mode can only be run as root.
2
+ exit 2
+ fi
+
+ download=1
+ ;;
+ p) silent=0 ;;
+ \?) usage ;;
+ esac
+done
if [[ -z $CHECKUPDATES_DB ]]; then
CHECKUPDATES_DB=${TMPDIR:-/tmp}/checkup-db-${USER}/
@@ -43,9 +70,17 @@ eval $(awk -F' *= *' '$1 ~ /DBPath/ { print $1 = $2 }'
@sysconfdir@/pacman.con
mkdir -p $CHECKUPDATES_DB
ln -s ${DBPath}/local $CHECKUPDATES_DB /dev/null
-fakeroot pacman -Sy --dbpath $CHECKUPDATES_DB --logfile /dev/null
/dev/null
+run_silent fakeroot pacman -Sy --dbpath $CHECKUPDATES_DB --logfile
/dev/null
pacman -Qqu --dbpath $CHECKUPDATES_DB 2 /dev/null
+if (( download )); then
+ if ! run_silent pacman -Suw --noconfirm --dbpath $CHECKUPDATES_DB;
then
+ ret=$?
+ (( silent )) echo Failed to download packages. 2
+ exit $ret
+ fi
+fi
+
exit 0
# vim: set ts=2 sw=2 noet:
Re: [pacman-dev] [PATCH] Put not explicitly installed packages in grey during update/install/removall
That colour cases the dependencies to stand out more on a terminal with a white background. I'd say bold would be better... ;) I agree on that part. I guess the best idea must be to create a color class for depend and for explicit so that changing it latter may be easier. However, the colour coding really is unclear. How do people come into the knowledge of what it means? For example, during an update I might think that a new package being pulled in as a dependency so it is highlighted. Or is it entirely obvious and I am thinking too hard? You are right. Perhaps a caption, like: --- $ sudo pacman -Su :: Starting full system upgrade... resolving dependencies... looking for inter-conflicts... Packages (21): **(Explict packages appears in bold)** Name Old Version New Version Net Change Download Size ... Total Download Size:154.72 MiB Total Installed Size: 491.30 MiB Net Upgrade Size: -1.28 MiB :: Proceed with installation? [Y/n] - ? (I had never though a *so simple* hack would generate so much discussion ;) -- Guillaume
Re: [pacman-dev] [PATCH v4 5/5] be_package: Build the file list from MTREE if possible
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 bluew...@xinu.at
---
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;
+}
+
Re: [pacman-dev] [PATCH] makepkg: remove unneeded || true
On Sun, Feb 02, 2014 at 05:39:18PM +1000, Allan McRae wrote:
makepkg only aborts on errors during PKGBUILD functions so the remaining
|| true statements are unneeded.
I think this probably extends to a lot of places we've used something
like '|| ret=$?' to get around this historical limitation.
Signed-off-by: Allan McRae al...@archlinux.org
---
scripts/makepkg.sh.in | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index ce6c9da..b69c071 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -1094,16 +1094,16 @@ remove_deps() {
# check for packages removed during dependency install (e.g. due to
conflicts)
# removing all installed packages is risky in this case
if [[ -n $(grep -xvFf (printf '%s\n' ${current_pkglist[@]}) \
- (printf '%s\n' ${original_pkglist[@]}) || true) ]];
then
+ (printf '%s\n' ${original_pkglist[@]})) ]]; then
warning $(gettext Failed to remove installed dependencies.)
return 0
fi
local deplist
deplist=($(grep -xvFf (printf %s\n ${original_pkglist[@]}) \
- (printf %s\n ${current_pkglist[@]}) || true))
+ (printf %s\n ${current_pkglist[@]})))
if [[ -z $deplist ]]; then
- return
+ return 0
fi
msg Removing installed dependencies...
@@ -2674,7 +2674,7 @@ fi
# set pacman command if not already defined
PACMAN=${PACMAN:-pacman}
# save full path to command as PATH may change when sourcing /etc/profile
-PACMAN_PATH=$(type -P $PACMAN) || true
+PACMAN_PATH=$(type -P $PACMAN)
# check if messages are to be printed using color
unset ALL_OFF BOLD BLUE GREEN RED YELLOW
--
1.8.5.3
Re: [pacman-dev] [PATCH v4 5/5] be_package: Build the file list from MTREE if possible
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 bluew...@xinu.at
---
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) {
+
Re: [pacman-dev] [PATCH] Put not explicitly installed packages in grey during update/install/removall
On Sun, Feb 2, 2014 at 8:45 AM, Guillaume Bouchard guillaume.bouch...@liris.cnrs.fr wrote: That colour cases the dependencies to stand out more on a terminal with a white background. I'd say bold would be better... ;) I agree on that part. I guess the best idea must be to create a color class for depend and for explicit so that changing it latter may be easier. However, the colour coding really is unclear. How do people come into the knowledge of what it means? For example, during an update I might think that a new package being pulled in as a dependency so it is highlighted. Or is it entirely obvious and I am thinking too hard? I agree that this would not be obvious to users. You are right. Perhaps a caption, like: --- $ sudo pacman -Su :: Starting full system upgrade... resolving dependencies... looking for inter-conflicts... Packages (21): **(Explict packages appears in bold)** Name Old Version New Version Net Change Download Size ... Total Download Size:154.72 MiB Total Installed Size: 491.30 MiB Net Upgrade Size: -1.28 MiB :: Proceed with installation? [Y/n] - ? (I had never though a *so simple* hack would generate so much discussion ;) -- Guillaume I don't like the idea of a caption that says something like explicitly installed packages appear in bold. An extra column would be better than a caption, but I don't know how everyone feels about that... I see three ways of doing this with a column: 1.) have a column title of Explicitly Installed? with a yes or no label for each package, optionally coloring the yes or no text for easy reading 2.) like the first way, but put an asterisk if the package is explicitly installed and leave it blank if the package is a dependency 3.) have a column title of Installed... with labels of explicitly or as a dependency Of those three, I think I prefer the second method. Jason
Re: [pacman-dev] [PATCH] Put not explicitly installed packages in grey during update/install/removall
On 03/02/14 15:34, Jason St. John wrote: On Sun, Feb 2, 2014 at 8:45 AM, Guillaume Bouchard guillaume.bouch...@liris.cnrs.fr wrote: That colour cases the dependencies to stand out more on a terminal with a white background. I'd say bold would be better... ;) I agree on that part. I guess the best idea must be to create a color class for depend and for explicit so that changing it latter may be easier. However, the colour coding really is unclear. How do people come into the knowledge of what it means? For example, during an update I might think that a new package being pulled in as a dependency so it is highlighted. Or is it entirely obvious and I am thinking too hard? I agree that this would not be obvious to users. You are right. Perhaps a caption, like: --- $ sudo pacman -Su :: Starting full system upgrade... resolving dependencies... looking for inter-conflicts... Packages (21): **(Explict packages appears in bold)** Name Old Version New Version Net Change Download Size ... Total Download Size:154.72 MiB Total Installed Size: 491.30 MiB Net Upgrade Size: -1.28 MiB :: Proceed with installation? [Y/n] - ? (I had never though a *so simple* hack would generate so much discussion ;) -- Guillaume I don't like the idea of a caption that says something like explicitly installed packages appear in bold. An extra column would be better than a caption, but I don't know how everyone feels about that... I see three ways of doing this with a column: 1.) have a column title of Explicitly Installed? with a yes or no label for each package, optionally coloring the yes or no text for easy reading 2.) like the first way, but put an asterisk if the package is explicitly installed and leave it blank if the package is a dependency 3.) have a column title of Installed... with labels of explicitly or as a dependency Of those three, I think I prefer the second method. I'm not sure there is enough room for another column - especially not with a title that long. Allan
