[pacman-dev] [PATCH] Handle null pkgcache for db_populate, refactor hash
Pang Yan Han
pangyanhan at gmail.com
Mon Feb 7 16:51:43 EST 2011
On Mon, Feb 07, 2011 at 09:44:39AM -0600, Dan McGee wrote:
> 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.
>
Hi yes I meant that the function will continue instead of bailing if
db->pkgcache happens to be NULL.
> > 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.
>
So I guess it's ok to set pmerrno to PM_ERR_MEMORY?
> >
> > 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.
>
Sure thing, I think it's neater that way too.
Thanks!
> > 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