[pacman-dev] [PATCH] Handle null pkgcache for db_populate, refactor hash
Dan McGee
dpmcgee at gmail.com
Mon Feb 7 10:44:39 EST 2011
On Mon, Feb 7, 2011 at 8:59 AM, Pang Yan Han <pangyanhan at gmail.com> wrote:
> In sync_db_populate() and local_db_populate(), a NULL
> db->pkgcache is not handled. This not only involves unnecessary
> operations, but also potentially merge sorting
> db->pkgcache->list which does not even exist in the case that
> db->pkgcache is NULL.
I'm not following the "unnecessary operations" part of this- do you
simply mean we would continue instead of bailing if the function
returned NULL? The only case that happens in would be if we ran out of
memory (MALLOC, CALLOC) or if we exceeded the maximum size.
> This may lead to serious issues since alpm_list_msort calls
> alpm_list_nth which will dereference invalid pointers.
> Thus, db->pkgcache is checked during local/sync db_populate
> calls.
>
> For this purpose, PM_ERR_DB_PKGCACHE_NULL error is added to
> _pmerrno_t and taken care of in alpm_strerror().
-1 on adding another error code that means nothing to a front-end
user. PM_ERR_MEMORY is already set in two of the three NULL exit
cases; we should set that in the third as well as greater than maximum
size might as well be a memory issue.
>
> In addition, common code between _alpm_pkghash_add() and
> _alpm_pkghash_add_sorted() is refactored out to
> _alpm_pkghash_add_pkg(). The 2 functions are now wrappers
> around _alpm_pkghash_add_pkg().
This belongs in a seperate patch, and I would prefer to not expose the
internal function, mark it static, and remove the need for a prototype
by pushing the implementors below the internal function.
> Signed-off-by: Pang Yan Han <pangyanhan at gmail.com>
> ---
> Please give me advice on this as to whether it needs tweaking
> or if it won't be accepted. Thanks!
>
> lib/libalpm/alpm.h | 1 +
> lib/libalpm/be_local.c | 4 ++++
> lib/libalpm/be_sync.c | 8 +++++++-
> lib/libalpm/db.c | 1 +
> lib/libalpm/error.c | 2 ++
> lib/libalpm/pkghash.c | 46 +++++++++++++++-------------------------------
> lib/libalpm/pkghash.h | 1 +
> 7 files changed, 31 insertions(+), 32 deletions(-)
>
> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> index 7fec293..57c68bd 100644
> --- a/lib/libalpm/alpm.h
> +++ b/lib/libalpm/alpm.h
> @@ -501,6 +501,7 @@ enum _pmerrno_t {
> PM_ERR_DB_NOT_FOUND,
> PM_ERR_DB_WRITE,
> PM_ERR_DB_REMOVE,
> + PM_ERR_DB_PKGCACHE_NULL,
> /* Servers */
> PM_ERR_SERVER_BAD_URL,
> PM_ERR_SERVER_NONE,
> diff --git a/lib/libalpm/be_local.c b/lib/libalpm/be_local.c
> index c5b2498..c7980f9 100644
> --- a/lib/libalpm/be_local.c
> +++ b/lib/libalpm/be_local.c
> @@ -393,6 +393,10 @@ static int local_db_populate(pmdb_t *db)
>
> /* initialize hash at 50% full */
> db->pkgcache = _alpm_pkghash_create(est_count * 2);
> + if(db->pkgcache == NULL){
> + closedir(dbdir);
> + RET_ERR(PM_ERR_DB_PKGCACHE_NULL, -1);
> + }
>
> while((ent = readdir(dbdir)) != NULL) {
> const char *name = ent->d_name;
> diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
> index 4ad045c..853ad30 100644
> --- a/lib/libalpm/be_sync.c
> +++ b/lib/libalpm/be_sync.c
> @@ -234,6 +234,9 @@ static int sync_db_populate(pmdb_t *db)
>
> /* initialize hash at 66% full */
> db->pkgcache = _alpm_pkghash_create(est_count * 3 / 2);
> + if(db->pkgcache == NULL) {
> + RET_ERR(PM_ERR_DB_PKGCACHE_NULL, -1);
> + }
>
> while(archive_read_next_header(archive, &entry) == ARCHIVE_OK) {
> const struct stat *st;
> @@ -348,6 +351,9 @@ static int sync_db_read(pmdb_t *db, struct archive *archive,
> if(likely_pkg && strcmp(likely_pkg->name, pkgname) == 0) {
> pkg = likely_pkg;
> } else {
> + if(db->pkgcache == NULL) {
> + RET_ERR(PM_ERR_DB_PKGCACHE_NULL, -1);
> + }
> pkg = _alpm_pkghash_find(db->pkgcache, pkgname);
> }
> if(pkg == NULL) {
> @@ -459,10 +465,10 @@ pmdb_t *_alpm_db_register_sync(const char *treename)
> _alpm_log(PM_LOG_DEBUG, "registering sync database '%s'\n", treename);
>
> db = _alpm_db_new(treename, 0);
> - db->ops = &sync_db_ops;
> if(db == NULL) {
> RET_ERR(PM_ERR_DB_CREATE, NULL);
> }
> + db->ops = &sync_db_ops;
>
> handle->dbs_sync = alpm_list_add(handle->dbs_sync, db);
> return(db);
> diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c
> index 02f8282..d43a16b 100644
> --- a/lib/libalpm/db.c
> +++ b/lib/libalpm/db.c
> @@ -370,6 +370,7 @@ pmdb_t *_alpm_db_new(const char *treename, int is_local)
> CALLOC(db, 1, sizeof(pmdb_t), RET_ERR(PM_ERR_MEMORY, NULL));
> STRDUP(db->treename, treename, RET_ERR(PM_ERR_MEMORY, NULL));
> db->is_local = is_local;
> + db->pkgcache_loaded = 0;
>
> return(db);
> }
> diff --git a/lib/libalpm/error.c b/lib/libalpm/error.c
> index 2cc6685..5b48c89 100644
> --- a/lib/libalpm/error.c
> +++ b/lib/libalpm/error.c
> @@ -81,6 +81,8 @@ const char SYMEXPORT *alpm_strerror(int err)
> return _("could not update database");
> case PM_ERR_DB_REMOVE:
> return _("could not remove database entry");
> + case PM_ERR_DB_PKGCACHE_NULL:
> + return _("could not initialize database package cache");
> /* Servers */
> case PM_ERR_SERVER_BAD_URL:
> return _("invalid url for server");
> diff --git a/lib/libalpm/pkghash.c b/lib/libalpm/pkghash.c
> index 5480527..7006888 100644
> --- a/lib/libalpm/pkghash.c
> +++ b/lib/libalpm/pkghash.c
> @@ -150,6 +150,16 @@ static pmpkghash_t *rehash(pmpkghash_t *oldhash)
>
> pmpkghash_t *_alpm_pkghash_add(pmpkghash_t *hash, pmpkg_t *pkg)
> {
> + return(_alpm_pkghash_add_pkg(hash, pkg, 0));
> +}
> +
> +pmpkghash_t *_alpm_pkghash_add_sorted(pmpkghash_t *hash, pmpkg_t *pkg)
> +{
> + return(_alpm_pkghash_add_pkg(hash, pkg, 1));
> +}
> +
> +pmpkghash_t *_alpm_pkghash_add_pkg(pmpkghash_t *hash, pmpkg_t *pkg, int sorted)
> +{
> alpm_list_t *ptr;
> size_t position;
>
> @@ -173,40 +183,13 @@ pmpkghash_t *_alpm_pkghash_add(pmpkghash_t *hash, pmpkg_t *pkg)
> ptr->prev = ptr;
>
> hash->hash_table[position] = ptr;
> - hash->list = alpm_list_join(hash->list, ptr);
> - hash->entries += 1;
> -
> - return(hash);
> -}
> -
> -pmpkghash_t *_alpm_pkghash_add_sorted(pmpkghash_t *hash, pmpkg_t *pkg)
> -{
> - if(!hash) {
> - return(_alpm_pkghash_add(hash, pkg));
> - }
> -
> - alpm_list_t *ptr;
> - size_t position;
> -
> - if((hash->entries + 1) / MAX_HASH_LOAD > hash->buckets) {
> - hash = rehash(hash);
> + if(!sorted){
> + hash->list = alpm_list_join(hash->list, ptr);
> + }else{
> + hash->list = alpm_list_mmerge(hash->list, ptr, _alpm_pkg_cmp);
> }
>
> - position = get_hash_position(pkg->name_hash, hash);
> -
> - ptr = calloc(1, sizeof(alpm_list_t));
> - if(ptr == NULL) {
> - return(hash);
> - }
> -
> - ptr->data = pkg;
> - ptr->next = NULL;
> - ptr->prev = ptr;
> -
> - hash->hash_table[position] = ptr;
> - hash->list = alpm_list_mmerge(hash->list, ptr, _alpm_pkg_cmp);
> hash->entries += 1;
> -
> return(hash);
> }
>
> @@ -344,3 +327,4 @@ pmpkg_t *_alpm_pkghash_find(pmpkghash_t *hash, const char *name)
>
> return(NULL);
> }
> +/* vim: set ts=2 sw=2 noet: */
> diff --git a/lib/libalpm/pkghash.h b/lib/libalpm/pkghash.h
> index a6c1db7..d63c6f0 100644
> --- a/lib/libalpm/pkghash.h
> +++ b/lib/libalpm/pkghash.h
> @@ -47,6 +47,7 @@ pmpkghash_t *_alpm_pkghash_create(size_t size);
>
> pmpkghash_t *_alpm_pkghash_add(pmpkghash_t *hash, pmpkg_t *pkg);
> pmpkghash_t *_alpm_pkghash_add_sorted(pmpkghash_t *hash, pmpkg_t *pkg);
> +pmpkghash_t *_alpm_pkghash_add_pkg(pmpkghash_t *hash, pmpkg_t *pkg, int sorted);
> pmpkghash_t *_alpm_pkghash_remove(pmpkghash_t *hash, pmpkg_t *pkg, pmpkg_t **data);
>
> void _alpm_pkghash_free(pmpkghash_t *hash);
> --
> 1.7.4
>
>
>
More information about the pacman-dev
mailing list