On Mon, Oct 12, 2009 at 3:03 PM, Henning Garus
On Sun, Oct 11, 2009 at 03:42:50PM +0200, Xavier Chantry wrote:
+/* create list of directories in db */ +static alpm_list_t *dirlist_from_fs(const char *syncdbpath) +{ + DIR *dbdir; + struct dirent *ent = NULL; + alpm_list_t *dirlist = NULL; + struct stat sbuf; + char path[PATH_MAX]; + + dbdir = opendir(syncdbpath); + if (dbdir != NULL) { + while((ent = readdir(dbdir)) != NULL) { + char *name = ent->d_name; + if(strcmp(name, ".") == 0 || strcmp(name, "..") == 0) { + continue; + } + + /* stat the entry, make sure it's a directory */ + snprintf(path, PATH_MAX, "%s%s", syncdbpath, name); + if(stat(path, &sbuf) != 0 || !S_ISDIR(sbuf.st_mode)) { + continue; + } + + int len = strlen(name); + char *entry = malloc(len + 2);
Isn't MALLOC more appropriate here?
Probably :)
+ strcpy(entry, name); + entry[len] = '/'; + entry[len+1] = '\0'; + dirlist = alpm_list_add(dirlist, entry); + } + } + closedir(dbdir); + + dirlist = alpm_list_msort(dirlist, alpm_list_count(dirlist), _alpm_str_cmp); + return(dirlist); +} + +/* remove old directories from dbdir */ +static int remove_olddir(const char *syncdbpath, alpm_list_t *dirlist) +{ + alpm_list_t *i; + for (i = dirlist; i; i = i->next) { + const char *name = i->data; + char *dbdir; + int len = strlen(syncdbpath) + strlen(name) + 2; + MALLOC(dbdir, len, RET_ERR(PM_ERR_MEMORY, -1)); + snprintf(dbdir, len, "%s%s", syncdbpath, name); + _alpm_log(PM_LOG_DEBUG, "removing: %s\n", dbdir); + if(_alpm_rmrf(dbdir) != 0) { + _alpm_log(PM_LOG_ERROR, _("could not remove database directory %s\n"), dbdir); + free(dbdir); + RET_ERR(PM_ERR_DB_REMOVE, -1); + } + free(dbdir); + } + return(0); +} + /** Update a package database * * An update of the package database \a db will be attempted. Unless @@ -229,27 +329,54 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db) return(-1); } else { const char *syncdbpath = _alpm_db_path(db); - /* remove the old dir */ - if(_alpm_rmrf(syncdbpath) != 0) { - _alpm_log(PM_LOG_ERROR, _("could not remove database %s\n"), db->treename); - RET_ERR(PM_ERR_DB_REMOVE, -1); - } - - /* Cache needs to be rebuilt */ - _alpm_db_free_pkgcache(db);
/* form the path to the db location */ len = strlen(dbpath) + strlen(db->treename) + strlen(DBEXT) + 1; MALLOC(dbfilepath, len, RET_ERR(PM_ERR_MEMORY, -1)); sprintf(dbfilepath, "%s%s" DBEXT, dbpath, db->treename);
- /* uncompress the sync database */ - checkdbdir(db); - ret = _alpm_unpack(dbfilepath, syncdbpath, NULL, 0); - if(ret) { - free(dbfilepath); - RET_ERR(PM_ERR_SYSTEM, -1); + /* remove the old dir if forcing update */ + if(force) { + if(_alpm_rmrf(syncdbpath) != 0) { + _alpm_log(PM_LOG_ERROR, _("could not remove database %s\n"), db->treename); + RET_ERR(PM_ERR_DB_REMOVE, -1); + } + + checkdbdir(db); + + ret = _alpm_unpack(dbfilepath, syncdbpath, NULL, 0); + if(ret) { + free(dbfilepath); + RET_ERR(PM_ERR_SYSTEM, -1); + } + } else { + alpm_list_t *onlyold = NULL; + alpm_list_t *onlynew = NULL; + alpm_list_t *olddirlist = NULL; + alpm_list_t *newdirlist = NULL; + + newdirlist = dirlist_from_tar(dbfilepath); + olddirlist = dirlist_from_fs(syncdbpath);
dirlist_from_tar could return NULL. dirlist_from_fs could as well if you error out on failing malloc (see above). I assume just moving on after failing to read the archive isn't really intended, since onlyold will contain all entries from olddirlist and onlynew will be empty, this would lead to an empty db.
Hmm. I think that these two functions can return NULL also in a non-error case. The sync database in the filesystem could be empty. And the new sync database tarball could be empty too (actually we just patched repo-add to support that). But yeah I will probably change the signature of these functions to distinguish error and empty case.
+ + alpm_list_diff_sorted(olddirlist, newdirlist, _alpm_str_cmp, &onlyold, &onlynew); + + ret = remove_olddir(syncdbpath, onlyold); + if(ret == 0) { + checkdbdir(db); + ret = _alpm_unpack(dbfilepath, syncdbpath, onlynew, 0); + } + + alpm_list_free(olddirlist); + alpm_list_free(newdirlist); + FREELIST(onlyold); + FREELIST(onlynew);
Shouldn't this be the other way round? onlyold and onlynew contain a subset of the entries in olddirlist and newdirlist.
Yeah, that seems right too :P Thanks for your review ! I will send a new patch ... when I can :)