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 <[email protected]> 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 <[email protected]> > > --- > > 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 > > > > > >
