[pacman-dev] [PATCH] Remove need to explicitly register the local DB
There is rarely a reason to ever not set up the local DB when getting a libalpm handle, so just do it automatically at this time. It is still a lazy initialization anyway, so there should be little to no fallout from doing it this way. Signed-off-by: Dan McGee <dan@archlinux.org> --- lib/libalpm/alpm.c | 4 ++++ lib/libalpm/alpm.h | 2 -- lib/libalpm/be_local.c | 7 +------ lib/libalpm/db.c | 15 --------------- src/pacman/database.c | 4 ++-- src/pacman/pacman.c | 9 --------- src/pacman/query.c | 11 +++++++++-- src/pacman/remove.c | 2 -- src/pacman/sync.c | 11 ++++++----- src/util/pactree.c | 2 +- src/util/testdb.c | 2 +- 11 files changed, 24 insertions(+), 45 deletions(-) diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c index 2a9f460..7c3bfc2 100644 --- a/lib/libalpm/alpm.c +++ b/lib/libalpm/alpm.c @@ -54,6 +54,10 @@ int SYMEXPORT alpm_initialize(void) if(handle == NULL) { RET_ERR(PM_ERR_MEMORY, -1); } + if(_alpm_db_register_local() == NULL) { + /* error code should be set */ + return(-1); + } #ifdef ENABLE_NLS bindtextdomain("libalpm", LOCALEDIR); diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index a540bc4..95482f0 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -174,8 +174,6 @@ typedef enum _pmpkgreason_t { * Databases */ -/* Preferred interfaces db_register_local and db_register_sync */ -pmdb_t *alpm_db_register_local(void); pmdb_t *alpm_db_register_sync(const char *treename); int alpm_db_unregister(pmdb_t *db); int alpm_db_unregister_all(void); diff --git a/lib/libalpm/be_local.c b/lib/libalpm/be_local.c index ea59cec..9602c82 100644 --- a/lib/libalpm/be_local.c +++ b/lib/libalpm/be_local.c @@ -400,7 +400,7 @@ static int local_db_populate(pmdb_t *db) pkg = _alpm_pkg_new(); if(pkg == NULL) { closedir(dbdir); - return(-1); + RET_ERR(PM_ERR_MEMORY, -1); } /* split the db entry name */ if(_alpm_splitname(name, pkg) != 0) { @@ -900,11 +900,6 @@ pmdb_t *_alpm_db_register_local(void) ALPM_LOG_FUNC; - if(handle->db_local != NULL) { - _alpm_log(PM_LOG_WARNING, _("attempt to re-register the 'local' DB\n")); - RET_ERR(PM_ERR_DB_NOT_NULL, NULL); - } - _alpm_log(PM_LOG_DEBUG, "registering local database\n"); db = _alpm_db_new("local", 1); diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c index bf9a70d..c80dcbb 100644 --- a/lib/libalpm/db.c +++ b/lib/libalpm/db.c @@ -64,21 +64,6 @@ pmdb_t SYMEXPORT *alpm_db_register_sync(const char *treename) return(_alpm_db_register_sync(treename)); } -/** Register the local package database. - * @return a pmdb_t* representing the local database, or NULL on error - */ -pmdb_t SYMEXPORT *alpm_db_register_local(void) -{ - ALPM_LOG_FUNC; - - /* Sanity checks */ - ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, NULL)); - /* Do not register a database if a transaction is on-going */ - ASSERT(handle->trans == NULL, RET_ERR(PM_ERR_TRANS_NOT_NULL, NULL)); - - return(_alpm_db_register_local()); -} - /* Helper function for alpm_db_unregister{_all} */ void _alpm_db_unregister(pmdb_t *db) { diff --git a/src/pacman/database.c b/src/pacman/database.c index 5fd33ea..36433f3 100644 --- a/src/pacman/database.c +++ b/src/pacman/database.c @@ -31,8 +31,6 @@ #include "conf.h" #include "util.h" -extern pmdb_t *db_local; - /** * @brief Modify the 'local' package database. * @@ -43,6 +41,7 @@ extern pmdb_t *db_local; int pacman_database(alpm_list_t *targets) { alpm_list_t *i; + pmdb_t *db_local; int retval = 0; pmpkgreason_t reason; @@ -65,6 +64,7 @@ int pacman_database(alpm_list_t *targets) return(1); } + db_local = alpm_option_get_localdb(); for(i = targets; i; i = alpm_list_next(i)) { char *pkgname = i->data; if(alpm_db_set_pkgreason(db_local, pkgname, reason) == -1) { diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index c267060..45500cf 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -57,7 +57,6 @@ #include "conf.h" #include "package.h" -pmdb_t *db_local; /* list of targets specified on command line */ static alpm_list_t *pm_targets; @@ -1433,14 +1432,6 @@ int main(int argc, char *argv[]) list_display("Targets :", pm_targets); } - /* Opening local database */ - db_local = alpm_db_register_local(); - if(db_local == NULL) { - pm_printf(PM_LOG_ERROR, _("could not register 'local' database (%s)\n"), - alpm_strerrorlast()); - cleanup(EXIT_FAILURE); - } - /* Log commandline */ if(needs_root()) { cl_to_log(argc, argv); diff --git a/src/pacman/query.c b/src/pacman/query.c index fc6a2a5..c79133d 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -37,8 +37,6 @@ #include "conf.h" #include "util.h" -extern pmdb_t *db_local; - static char *resolve_path(const char *file) { char *str = NULL; @@ -102,6 +100,7 @@ static int query_fileowner(alpm_list_t *targets) char *append; size_t max_length; alpm_list_t *t; + pmdb_t *db_local; /* This code is here for safety only */ if(targets == NULL) { @@ -117,6 +116,8 @@ static int query_fileowner(alpm_list_t *targets) append = path + strlen(path); max_length = PATH_MAX - (append - path) - 1; + db_local = alpm_option_get_localdb(); + for(t = targets; t; t = alpm_list_next(t)) { char *filename, *dname, *rpath; const char *bname; @@ -220,6 +221,7 @@ static int query_search(alpm_list_t *targets) { alpm_list_t *i, *searchlist; int freelist; + pmdb_t *db_local = alpm_option_get_localdb(); /* if we have a targets list, search for packages matching it */ if(targets) { @@ -286,6 +288,8 @@ static int query_group(alpm_list_t *targets) alpm_list_t *i, *j; char *grpname = NULL; int ret = 0; + pmdb_t *db_local = alpm_option_get_localdb(); + if(targets == NULL) { for(j = alpm_db_get_grpcache(db_local); j; j = alpm_list_next(j)) { pmgrp_t *grp = alpm_list_getdata(j); @@ -471,6 +475,7 @@ int pacman_query(alpm_list_t *targets) int match = 0; alpm_list_t *i; pmpkg_t *pkg = NULL; + pmdb_t *db_local; /* First: operations that do not require targets */ @@ -495,6 +500,8 @@ int pacman_query(alpm_list_t *targets) } } + db_local = alpm_option_get_localdb(); + /* operations on all packages in the local DB * valid: no-op (plain -Q), list, info, check * invalid: isfile, owns */ diff --git a/src/pacman/remove.c b/src/pacman/remove.c index 52f92ec..82d1c38 100644 --- a/src/pacman/remove.c +++ b/src/pacman/remove.c @@ -31,8 +31,6 @@ #include "util.h" #include "conf.h" -extern pmdb_t *db_local; - /** * @brief Remove a specified list of packages. * diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 278f15e..7353f7e 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -37,8 +37,6 @@ #include "package.h" #include "conf.h" -extern pmdb_t *db_local; - /* if keep_used != 0, then the db files which match an used syncdb * will be kept */ static int sync_cleandb(const char *dbpath, int keep_used) { @@ -144,6 +142,7 @@ static int sync_cleancache(int level) { alpm_list_t *i; alpm_list_t *sync_dbs = alpm_option_get_syncdbs(); + pmdb_t *db_local = alpm_option_get_localdb(); int ret = 0; for(i = alpm_option_get_cachedirs(); i; i = alpm_list_next(i)) { @@ -295,7 +294,7 @@ static int sync_synctree(int level, alpm_list_t *syncs) return(success > 0); } -static void print_installed(pmpkg_t *pkg) +static void print_installed(pmdb_t *db_local, pmpkg_t *pkg) { const char *pkgname = alpm_pkg_get_name(pkg); const char *pkgver = alpm_pkg_get_version(pkg); @@ -316,6 +315,7 @@ static int sync_search(alpm_list_t *syncs, alpm_list_t *targets) alpm_list_t *i, *j, *ret; int freelist; int found = 0; + pmdb_t *db_local = alpm_option_get_localdb(); for(i = syncs; i; i = alpm_list_next(i)) { pmdb_t *db = alpm_list_getdata(i); @@ -366,7 +366,7 @@ static int sync_search(alpm_list_t *syncs, alpm_list_t *targets) printf(")"); } - print_installed(pkg); + print_installed(db_local, pkg); /* we need a newline and initial indent first */ printf("\n "); @@ -519,6 +519,7 @@ static int sync_info(alpm_list_t *syncs, alpm_list_t *targets) static int sync_list(alpm_list_t *syncs, alpm_list_t *targets) { alpm_list_t *i, *j, *ls = NULL; + pmdb_t *db_local = alpm_option_get_localdb(); if(targets) { for(i = targets; i; i = alpm_list_next(i)) { @@ -556,7 +557,7 @@ static int sync_list(alpm_list_t *syncs, alpm_list_t *targets) if (!config->quiet) { printf("%s %s %s", alpm_db_get_name(db), alpm_pkg_get_name(pkg), alpm_pkg_get_version(pkg)); - print_installed(pkg); + print_installed(db_local, pkg); printf("\n"); } else { printf("%s\n", alpm_pkg_get_name(pkg)); diff --git a/src/util/pactree.c b/src/util/pactree.c index 0ac3f24..6a10006 100644 --- a/src/util/pactree.c +++ b/src/util/pactree.c @@ -77,7 +77,7 @@ static int alpm_local_init(void) return(ret); } - db_local = alpm_db_register_local(); + db_local = alpm_option_get_localdb(); if(!db_local) { return(1); } diff --git a/src/util/testdb.c b/src/util/testdb.c index 96a123a..461cf23 100644 --- a/src/util/testdb.c +++ b/src/util/testdb.c @@ -136,7 +136,7 @@ static int check_localdb(void) { return(ret); } - db = alpm_db_register_local(); + db = alpm_option_get_localdb(); if(db == NULL) { fprintf(stderr, "error: could not register 'local' database (%s)\n", alpm_strerrorlast()); -- 1.7.3.5
Dan McGee wrote:
There is rarely a reason to ever not set up the local DB when getting a libalpm handle, so just do it automatically at this time. It is still a lazy initialization anyway, so there should be little to no fallout from doing it this way.
I'm concerned about the uncertainty of "little to no fallout". What exactly does the "lazy" initialization do? Does it in any way rely on the presence of a local database? I ask because I have a tool in mind that will use ALPM to query the sync database, most likely in the absence of a local database. It also strikes me as bad design to hard-code something that is not necessary for all operations, especially for a library that is at least intended for use beyond Pacman. Wouldn't it make more sense to create a separate routine to automatically set up the local DB when getting a libalpm handle? Applications that need the local DB could just use that instead wherever they acquire the handle.
On 29/01/11 14:51, Xyne wrote:
Dan McGee wrote:
There is rarely a reason to ever not set up the local DB when getting a libalpm handle, so just do it automatically at this time. It is still a lazy initialization anyway, so there should be little to no fallout from doing it this way.
I'm concerned about the uncertainty of "little to no fallout". What exactly does the "lazy" initialization do? Does it in any way rely on the presence of a local database?
The lazy initialisation means that nothing is actually loaded when registering the local database. The local database is only read in as needed and only the parts that are needed are read in. In fact, all that is done registering the local database is creating a pmdb_t srtuct and assigning the local database operation struct to it. That is the "little to no fallout" if you do not use the local database. Allan
Allan McRae wrote:
The lazy initialisation means that nothing is actually loaded when registering the local database. The local database is only read in as needed and only the parts that are needed are read in.
In fact, all that is done registering the local database is creating a pmdb_t srtuct and assigning the local database operation struct to it. That is the "little to no fallout" if you do not use the local database.
Allan
Thanks for the clarification. Even if it is negligible, it's munging separate logic together. Given that the purpose is presumably to save coders a few lines of code, I still think it would make more sense to use a wrapper function to do that when getting the handle. Don't worry though, I'll leave it there as I have no intention of pestering you about something that is effectively irrelevant. I just see it as lazy design in a bad way. Regards, Xyne
On Sat, Jan 29, 2011 at 3:09 AM, Xyne <xyne@archlinux.ca> wrote:
Allan McRae wrote:
The lazy initialisation means that nothing is actually loaded when registering the local database. The local database is only read in as needed and only the parts that are needed are read in.
In fact, all that is done registering the local database is creating a pmdb_t srtuct and assigning the local database operation struct to it. That is the "little to no fallout" if you do not use the local database.
Allan
Thanks for the clarification.
Even if it is negligible, it's munging separate logic together. Given that the purpose is presumably to save coders a few lines of code, I still think it would make more sense to use a wrapper function to do that when getting the handle.
The "register" code could just as well be done in the handle setup where I moved the call anyway. You'd save about 13 CPU cycles. It's design that is meant to remove stupidity in a library where you have to make what seems like a no-op call- but make sure you only do it once. Why let users shoot themselves in the foot? We already handled the teardown in alpm_release so this just makes the initialize side map that. -Dan -Dan
On Sat, Jan 29, 2011 at 9:59 AM, Dan McGee <dpmcgee@gmail.com> wrote:
-Dan
-Dan
^^ A sign I need my morning coffee... :)
On Sat, Jan 29, 2011 at 10:09 AM, Xyne <xyne@archlinux.ca> wrote:
Even if it is negligible, it's munging separate logic together. Given that the purpose is presumably to save coders a few lines of code, I still think it would make more sense to use a wrapper function to do that when getting the handle.
It's not separate logic. Dan commit log might have been a bit misleading. alpm_initialize initializes (wow) whatever is needed for the other functions to work. What we do there is just allocate some structures like the handle, and now handle->db_local as well. We are talking about one calloc and one strdup here to setup db_local. Really no big deal, there is no loading of the db, not even a check that one exists at all. And it does make it simpler for libalpm users. And as Dan said its symetric with alpm_release.
Dan McGee wrote:
The "register" code could just as well be done in the handle setup where I moved the call anyway. You'd save about 13 CPU cycles. It's design that is meant to remove stupidity in a library where you have to make what seems like a no-op call- but make sure you only do it once. Why let users shoot themselves in the foot? We already handled the teardown in alpm_release so this just makes the initialize side map that.
Xavier Chantry wrote:
It's not separate logic. Dan commit log might have been a bit misleading.
alpm_initialize initializes (wow) whatever is needed for the other functions to work. What we do there is just allocate some structures like the handle, and now handle->db_local as well. We are talking about one calloc and one strdup here to setup db_local. Really no big deal, there is no loading of the db, not even a check that one exists at all. And it does make it simpler for libalpm users. And as Dan said its symetric with alpm_release.
Ah. It makes much more sense now, especially given the symmetry with alpm_release. Thanks.
-Dan
-Dan
^^ A sign I need my morning coffee... :)
I just assumed that you had graduated to Double Dan: https://secure.wikimedia.org/wikipedia/en/wiki/Dan_%28rank%29
participants (5)
-
Allan McRae
-
Dan McGee
-
Dan McGee
-
Xavier Chantry
-
Xyne