[pacman-dev] Long functions and other minor fixes
Some more functions that are worth taking a look at on the libalpm side: add.c: _alpm_add_commit be_files.c: _alpm_db_read _alpm_db_write conflict.c: _alpm_checkconflicts _alpm_db_find_conflicts package.c: _alpm_pkg_load (getting a bit long) remove.c: _alpm_remove_commit server.c: _alpm_downloadfiles_forreal sync.c: _alpm_sync_sysupgrade (getting there) _alpm_sync_prepare _alpm_sync_commit And some minor things I found in the code- cleaned up an extern enum, changed 'md5' variable to 'hash' as it can be either md5 or sha1, resorted db.h for clarity, and fixed some English grammar issues. Here is a patch: Index: lib/libalpm/alpm.h =================================================================== RCS file: /home/cvs-pacman/pacman-lib/lib/libalpm/alpm.h,v retrieving revision 1.64 diff -u -u -r1.64 alpm.h --- lib/libalpm/alpm.h 22 Nov 2006 09:03:41 -0000 1.64 +++ lib/libalpm/alpm.h 22 Dec 2006 18:34:01 -0000 @@ -399,7 +399,7 @@ /* * Errors */ -extern enum __pmerrno_t { +enum __pmerrno_t { PM_ERR_MEMORY = 1, PM_ERR_SYSTEM, PM_ERR_BADPERMS, @@ -469,7 +469,9 @@ /* Downloading */ PM_ERR_CONNECT_FAILED, PM_ERR_FORK_FAILED -} pm_errno; +}; + +extern enum __pmerrno_t pm_errno; char *alpm_strerror(int err); Index: lib/libalpm/backup.c =================================================================== RCS file: /home/cvs-pacman/pacman-lib/lib/libalpm/backup.c,v retrieving revision 1.7 diff -u -u -r1.7 backup.c --- lib/libalpm/backup.c 31 Oct 2006 06:39:59 -0000 1.7 +++ lib/libalpm/backup.c 22 Dec 2006 18:34:01 -0000 @@ -55,9 +55,9 @@ ptr++; /* now str points to the filename and ptr points to the md5 or sha1 hash */ if(!strcmp(file, str)) { - char *md5 = strdup(ptr); + char *hash = strdup(ptr); FREE(str); - return(md5); + return(hash); } FREE(str); } Index: lib/libalpm/db.h =================================================================== RCS file: /home/cvs-pacman/pacman-lib/lib/libalpm/db.h,v retrieving revision 1.20 diff -u -u -r1.20 db.h --- lib/libalpm/db.h 20 Nov 2006 09:10:24 -0000 1.20 +++ lib/libalpm/db.h 22 Dec 2006 18:34:01 -0000 @@ -44,10 +44,14 @@ pmlist_t *servers; }; +/* db.c, database general calls */ pmdb_t *_alpm_db_new(char *root, char *dbpath, char *treename); void _alpm_db_free(void *data); int _alpm_db_cmp(const void *db1, const void *db2); pmlist_t *_alpm_db_search(pmdb_t *db, pmlist_t *needles); +pmdb_t *_alpm_db_register(char *treename, alpm_cb_db_register callback); + +/* be.c, backend specific */ int _alpm_db_install(pmdb_t *db, const char *dbfile); int _alpm_db_open(pmdb_t *db); void _alpm_db_close(pmdb_t *db); @@ -58,7 +62,6 @@ int _alpm_db_remove(pmdb_t *db, pmpkg_t *info); int _alpm_db_getlastupdate(pmdb_t *db, char *ts); int _alpm_db_setlastupdate(pmdb_t *db, char *ts); -pmdb_t *_alpm_db_register(char *treename, alpm_cb_db_register callback); #endif /* _ALPM_DB_H */ Index: lib/libalpm/sync.c =================================================================== RCS file: /home/cvs-pacman/pacman-lib/lib/libalpm/sync.c,v retrieving revision 1.85 diff -u -u -r1.85 sync.c --- lib/libalpm/sync.c 1 Dec 2006 09:32:30 -0000 1.85 +++ lib/libalpm/sync.c 22 Dec 2006 18:34:03 -0000 @@ -382,7 +382,7 @@ { pmlist_t *deps = NULL; pmlist_t *list = NULL; /* list allowing checkdeps usage with data from trans->packages */ - pmlist_t *trail = NULL; /* breadcrum list to avoid running into circles */ + pmlist_t *trail = NULL; /* breadcrumb list to avoid running in circles */ pmlist_t *asked = NULL; pmlist_t *i, *j, *k, *l; int ret = 0;
On 12/22/06, Dan McGee <dpmcgee@gmail.com> wrote:
Some more functions that are worth taking a look at on the libalpm side: add.c: _alpm_add_commit be_files.c: _alpm_db_read _alpm_db_write conflict.c: _alpm_checkconflicts _alpm_db_find_conflicts package.c: _alpm_pkg_load (getting a bit long) remove.c: _alpm_remove_commit server.c: _alpm_downloadfiles_forreal sync.c: _alpm_sync_sysupgrade (getting there) _alpm_sync_prepare _alpm_sync_commit
Thanks for the list! I will look at these this weekend, as I won't really be available (holidays and all) that much.
And some minor things I found in the code- cleaned up an extern enum, changed 'md5' variable to 'hash' as it can be either md5 or sha1, resorted db.h for clarity, and fixed some English grammar issues. Here is a patch:
Applied. Thanks!
This is a refactoring for _alpm_sync_sysupgrade, but I didn't really cut down on the total code length. I just thought the triple nested for loops deserved their own function (and maybe this will lead to inspiration to cleaning up this code). The patch looks kind of weird but a vimdiff makes a lot more sense of it. -Dan $ cvs diff lib/libalpm/sync.c Index: lib/libalpm/sync.c =================================================================== RCS file: /home/cvs-pacman/pacman-lib/lib/libalpm/sync.c,v retrieving revision 1.86 diff -u -r1.86 sync.c --- lib/libalpm/sync.c 22 Dec 2006 19:38:55 -0000 1.86 +++ lib/libalpm/sync.c 28 Dec 2006 19:09:27 -0000 @@ -118,12 +118,14 @@ return((pkg->date + handle->upgradedelay) > t); } -int _alpm_sync_sysupgrade(pmtrans_t *trans, pmdb_t *db_local, pmlist_t *dbs_sync) +/* Find recommended replacements for packages during a sync. + * (refactored from _alpm_sync_prepare) + */ +static int find_replacements(pmtrans_t *trans, pmdb_t *db_local, pmlist_t *dbs_sync) { pmlist_t *i, *j, *k; - /* check for "recommended" package replacements */ - _alpm_log(PM_LOG_FLOW1, _("checking for package replacements")); + /* TODO can these nested loops be commented better? */ for(i = dbs_sync; i; i = i->next) { for(j = _alpm_db_get_pkgcache(i->data, INFRQ_DESC); j; j = j->next) { pmpkg_t *spkg = j->data; @@ -179,6 +181,23 @@ } } + return(0); + +error: + return(-1); +} + +int _alpm_sync_sysupgrade(pmtrans_t *trans, pmdb_t *db_local, pmlist_t *dbs_sync) +{ + pmlist_t *i, *j; + + /* check for "recommended" package replacements */ + _alpm_log(PM_LOG_FLOW1, _("checking for package replacements")); + if( find_replacements(trans, db_local, dbs_sync) != 0 ) { + /* pm_errno is set by find_replacement */ + goto error; + } + /* match installed packages with the sync dbs and compare versions */ _alpm_log(PM_LOG_FLOW1, _("checking for package upgrades")); for(i = _alpm_db_get_pkgcache(db_local, INFRQ_NONE); i; i = i->next) { @@ -382,7 +401,7 @@ { pmlist_t *deps = NULL; pmlist_t *list = NULL; /* list allowing checkdeps usage with data from trans->packages */ - pmlist_t *trail = NULL; /* breadcrumb list to avoid running into circles */ + pmlist_t *trail = NULL; /* breadcrumb list to avoid running in circles */ pmlist_t *asked = NULL; pmlist_t *i, *j, *k, *l; int ret = 0;
On 12/28/06, Dan McGee <dpmcgee@gmail.com> wrote:
This is a refactoring for _alpm_sync_sysupgrade, but I didn't really cut down on the total code length. I just thought the triple nested for loops deserved their own function (and maybe this will lead to inspiration to cleaning up this code). The patch looks kind of weird but a vimdiff makes a lot more sense of it.
This is great! Exactly what I'm looking to do. I've applied it, but made a few minor changes (while I like goto in some cases, it can be factored out in the above code, so I did so). Another important rule of thumb was best said by Linus himself: If you need more than three levels of indentation, you're screwed anyway, and should fix your program. All gargantuan nested things are just... too much. It's too hard to follow, and this prevents people from contributing. For those interested parties who don't know what I'm talking about, here's an example of how something like this would work (though not really in the spirit of what linus is saying): for(int i...) { for(int j...) { for(int k...) { for(int l...) { } } } } can become: void do_something(int i, int j) { for(int k...) { for(int l...) { } } } for(int i...) { for(int j...) { do_something(i, j); } } While this is not EXACTLY what the "three levels of indentation" adage is saying, this is a start to refactoring some of this stuff. On a side note, alot of these functions do things like "find_package(db, pkgname)" in a loop over all DBs. To me, it makes much more sense to just do "find_package(pkgname)" and let that function find the appropriate DB. The package structure does actually contain a pointer to the DB (if it's a sync package... kinda nasty, but it works well enough), so we can back-reference the DB once it's found if we need to. Alot of these (db, pkg) functions can be cleaned up in this case, assuming there are proper checks to verify we're looking at a sync package. So, for anyone interested, I still have a bunch of bugtracker bugs (17 at my last count, excluding FRs), and need to get those together, but I will be working on refactoring some of this stuff as I touch things too. If anyone is interested in doing some code cleanup (yeah, it's a thankless job) let me know. I'd like to organize this so we don't step on each other's toes. Also, feel free to check out the bug reports. There appears to be 3-4 that have to do with dep tracking.
On 12/29/06, Aaron Griffin <aaronmgriffin@gmail.com> > So, for anyone interested, I still have a bunch of bugtracker bugs (17
at my last count, excluding FRs), and need to get those together Make that 33 in total, not all of them are assigned to me: http://bugs.archlinux.org/index.php?project=1&type=1&cat=6
participants (2)
-
Aaron Griffin
-
Dan McGee