reassign 562398 libdebian-installer4-udeb found 562398 0.69 retitle 562398 libdebian-installer: strange behavior with more than one version of a package in a Packages file affects 562398 + anna tags 562398 + patch thanks
On Sun, May 02, 2010 at 03:08:08PM -0600, John Wright wrote: > Specifically, what happens is that anna unpacks all the packages in one > batch, and then it configures all of them. But while unpacking another > version of a package while another vesrion is in an > unpacked-but-not-configured state is ok, it's not ok to configure a > package that's already in the configured state. So if a package is in > the list twice, it fails at the second configure for that package. Here's what's happing in libd-i: Upon encountering a Packages stanza with the same Package field as one it's previously seen, di_packages_parser_read_name sets the data pointer the rest of the parsing functions will use to the previously-used di_packages pointer. At first glance, this would appear simply to prefer packages that appear later in the Packages file, irrespective of version. Unfortunately, it appends the di_package to the package list (&parser->data->packages->list) whether it's a newly allocated one or an old one. So while only one actual di_package exists for a particular package name, it might appear multiple times in the list. The simple way to fix the anna issue is to make sure we only append newly allocated di_package objects to the list. But it would be nice to favor new versions rather than whatever shows up latest in the Packages file (for example, to facilitate #389430 or LP#234486). I've attached a quick reproducer to demonstrate the issue, and a patch. I would prefer if the version comparison could happen during the Packages file parsing, rather than after the fact (since this way requires creating a temporary hash table and traversing the list a couple of extra times), but that change would seem to be very invasive. In any case, after pounding my head for a couple of hours, I couldn't figure out how to do it any better with the current parsing infrastructure. :) -- John Wright <j...@debian.org>
#include <stdio.h> #include <debian-installer/system/packages.h> #include <debian-installer/slist.h> int main(int argc, char **argv) { di_packages_allocator *allocator = di_system_packages_allocator_alloc(); di_packages *packages; di_package *package, *last_package = NULL; di_slist_node *node; const char *packages_file; if (argc == 2) packages_file = argv[1]; else { printf("Usage: %s <packages-file>\n", argv[0]); return 1; } packages = di_system_packages_read_file(packages_file, allocator); for (node = packages->list.head; node; node = node->next) { package = node->data; if (!package) printf("package == NULL\n"); if (package == last_package) printf("Eek! package == last_package\n"); printf("Package: %s\n", package->package); printf("Version: %s\n", package->version); printf("\n"); last_package = package; } return 0; }
Package: baz Version: 0.0~hpde1 Package: bang Version: 0.133 Package: baz Version: 0.0hpde1 Package: foo Version: 1.2.3 Package: foo Version: 1.2.3hpde1 Package: bar Version: 2.3.4hpde1 Package: bar Version: 2.3.4 Package: baz Version: 0.1
>From 64d06247e4fdb35fd7f33eb7020ec84584e23b42 Mon Sep 17 00:00:00 2001 From: John Wright <j...@johnwright.org> Date: Sun, 2 May 2010 22:15:11 -0600 Subject: [PATCH] Filter out old versions after parsing a Packages file The previous behavior favored packages showing up later in the Packages file rather than packages with later versions, and wound up putting multiple entries for the same package in the di_packages list. This patch ensures a new di_package is created for each stanza, and then filters out all but the latest versions before returning the di_packages pointer. There's a small memory leak for each di_slist_node corresponding to an old package version. It can't be helped, as far as I can tell, because of how the memory for those are allocated (using mem_chunk). The bulk of the memory used for those is freed, just not the di_slist_node glue. This patch also fixes a bug in di_hash_table_insert, where the key was not being reset after potentially destroying the old key and changing the value for a key (which may contain the key itself). --- include/debian-installer/packages.h | 4 ++ src/hash.c | 1 + src/packages.c | 69 +++++++++++++++++++++++++++++++++++ src/packages_parser.c | 8 +++- 4 files changed, 80 insertions(+), 2 deletions(-) diff --git a/include/debian-installer/packages.h b/include/debian-installer/packages.h index c5e4576..0d7dd1c 100644 --- a/include/debian-installer/packages.h +++ b/include/debian-installer/packages.h @@ -71,6 +71,10 @@ void di_packages_append_package (di_packages *packages, di_package *package, di_ di_package *di_packages_get_package (di_packages *packages, const char *name, size_t n); di_package *di_packages_get_package_new (di_packages *packages, di_packages_allocator *allocator, char *name, size_t n); +di_hash_table *di_packages_get_latest_map (di_packages *packages); +void di_packages_filter_older_versions(di_packages *packages); +void di_packages_update_hash_table(di_packages *packages); + di_slist *di_packages_resolve_dependencies (di_packages *packages, di_slist *list, di_packages_allocator *allocator); di_slist *di_packages_resolve_dependencies_array (di_packages *packages, di_package **array, di_packages_allocator *allocator); void di_packages_resolve_dependencies_mark (di_packages *packages); diff --git a/src/hash.c b/src/hash.c index 302ea5a..78a2f5d 100644 --- a/src/hash.c +++ b/src/hash.c @@ -192,6 +192,7 @@ void di_hash_table_insert (di_hash_table *hash_table, void *key, void *value) if (hash_table->value_destroy_func) hash_table->value_destroy_func ((*node)->value); + (*node)->key = key; (*node)->value = value; } else diff --git a/src/packages.c b/src/packages.c index 699f82c..e0f9d59 100644 --- a/src/packages.c +++ b/src/packages.c @@ -164,6 +164,75 @@ di_package *di_packages_get_package_new (di_packages *packages, di_packages_allo return ret; } +di_hash_table *di_packages_get_latest_map (di_packages *packages) +{ + di_slist_node *node; + di_package *package, *latest_package; + di_package_version *this_version = NULL, *latest_version = NULL; + di_hash_table *latest = di_hash_table_new (di_rstring_hash, di_rstring_equal); + + for (node = packages->list.head; node; node = node->next) { + package = node->data; + latest_package = di_hash_table_lookup(latest, &package->key); + this_version = latest_version = NULL; + + /* With missing or broken versions, just take the last-added package */ + if (!latest_package || + !package->version || !latest_package->version || + !(this_version = di_package_version_parse (package)) || + !(latest_version = di_package_version_parse (latest_package)) || + di_package_version_compare (this_version, latest_version) > 0) + { + di_hash_table_insert (latest, &package->key, package); + di_free (this_version); + di_free (latest_version); + } + } + + return latest; +} + +void di_packages_filter_older_versions (di_packages *packages) +{ + di_slist_node *node, *last_node; + di_package *package, *latest_package; + di_hash_table *latest = di_packages_get_latest_map (packages); + + /* Remove non-latest packages from the list */ + last_node = NULL; + for (node = packages->list.head; node; node = node->next) { + package = node->data; + latest_package = di_hash_table_lookup (latest, &package->key); + if (package != latest_package) { + /* Remove it from the list */ + if (last_node) + last_node->next = node->next; + else + packages->list.head = node->next; + + if (!node->next) + packages->list.bottom = last_node; + + di_package_destroy(package); + } + else + last_node = node; + } + + di_hash_table_destroy (latest); +} + +void di_packages_update_hash_table (di_packages *packages) +{ + di_slist_node *node; + di_package *package; + + for (node = packages->list.head; node; node = node->next) { + package = node->data; + di_hash_table_insert (packages->table, &package->key, package); + } +} + bool di_packages_resolve_dependencies_recurse (di_packages_resolve_dependencies_check *r, di_package *package, di_package *dependend_package) { di_slist_node *node; diff --git a/src/packages_parser.c b/src/packages_parser.c index 5b4ccbd..24f303d 100644 --- a/src/packages_parser.c +++ b/src/packages_parser.c @@ -182,6 +182,9 @@ di_packages *di_packages_special_read_file (const char *file, di_packages_alloca di_parser_info_free (info); + di_packages_filter_older_versions (data.packages); + di_packages_update_hash_table (data.packages); + return data.packages; } @@ -240,8 +243,9 @@ void di_packages_parser_read_name (data, fip, field_modifier, value, user_data) void *user_data; { internal_di_package_parser_data *parser_data = user_data; - di_package *p; - p = di_packages_get_package_new (parser_data->packages, parser_data->allocator, value->string, value->size); + di_package *p = di_package_alloc (parser_data->allocator); + p->key.string = di_stradup (value->string, value->size); + p->key.size = value->size; p->type = di_package_type_real_package; di_slist_append_chunk (&parser_data->packages->list, p, parser_data->allocator->slist_node_mem_chunk); *data = p; -- 1.6.5