[pacman-dev] [PATCH 3/3] Only extract new db entries

[Xavier]

On Mon, Oct 12, 2009 at 3:03 PM, Henning Garus
<henning.garus at googlemail.com> wrote:
> 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 :)