Re: [pacman-dev] [PATCH] Get rid of double / in database paths.

On Mon, Jun 2, 2008 at 4:56 AM, Xavier  wrote:
> >From c69a3efc4d6e08ea967a936afc8f9622e1e6b268 Mon Sep 17 00:00:00 2001
> From: Xavier Chantry 
> Date: Fri, 30 May 2008 08:52:27 +0200
> Subject: [PATCH] Get rid of double / in database paths.
>
> Errors like the following one happen regularly (for unknown reasons...) :
> error: could not open file /var/lib/pacman/local//glibc-2.7-9/depends: No
> such file or directory
>
> Anyway, every time an user reported an error like that, it always seemed
> like he thought the error was caused by the double /, which is obviously
> wrong.
>
> Since db->path always include a trailing /, there is no need to add one when
> concatenating paths in be_files.c or add.c.
> Additionally, some static strings were switched to dynamic.
> And the computation of the "dbpath"/"pkgname"-"pkgversion" was refactored
> in db_read, db_write and db_remove with a get_pkgpath static function.
>
> Signed-off-by: Xavier Chantry 
> ---
>  lib/libalpm/add.c      |    4 +-
>  lib/libalpm/be_files.c |   98 ++++++++++++++++++++++++++++++++----------------
>  2 files changed, 68 insertions(+), 34 deletions(-)
>
> diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c
> index c5454ba..3d53a1c 100644
> --- a/lib/libalpm/add.c
> +++ b/lib/libalpm/add.c
> @@ -287,12 +287,12 @@ static int extract_single_file(struct archive *archive,
>
>        if(strcmp(entryname, ".INSTALL") == 0) {
>                /* the install script goes inside the db */
> -               snprintf(filename, PATH_MAX, "%s/%s-%s/install", db->path,
> +               snprintf(filename, PATH_MAX, "%s%s-%s/install", db->path,
>                                newpkg->name, newpkg->version);
>                archive_entry_set_mode(entry, 0644);
>        } else if(strcmp(entryname, ".CHANGELOG") == 0) {
>                /* the changelog goes inside the db */
> -               snprintf(filename, PATH_MAX, "%s/%s-%s/changelog", db->path,
> +               snprintf(filename, PATH_MAX, "%s%s-%s/changelog", db->path,
>                                newpkg->name, newpkg->version);
>                archive_entry_set_mode(entry, 0644);
>        } else if(*entryname == '.') {
> diff --git a/lib/libalpm/be_files.c b/lib/libalpm/be_files.c
> index 2302374..6eebab7 100644
> --- a/lib/libalpm/be_files.c
> +++ b/lib/libalpm/be_files.c
> @@ -56,6 +56,7 @@ time_t getlastupdate(const pmdb_t *db)
>        FILE *fp;
>        char *file;
>        time_t ret = 0;
> +       int len = 0;
>
>        ALPM_LOG_FUNC;
>
> @@ -64,8 +65,9 @@ time_t getlastupdate(const pmdb_t *db)
>        }
>
>        /* db->path + '.lastupdate' + NULL */
> -       MALLOC(file, strlen(db->path) + 12, RET_ERR(PM_ERR_MEMORY, ret));
> -       sprintf(file, "%s.lastupdate", db->path);
> +       len = strlen(db->path) + 12;
> +       CALLOC(file, len, sizeof(char), RET_ERR(PM_ERR_MEMORY, ret));
> +       snprintf(file, len, "%s.lastupdate", db->path);
These function switches aren't really necessary:
1. malloc -> calloc: we are filling the whole of the newly allocated
memory as soon as we get it, so really no need to zero it out
2. sprintf -> snprintf: no need for the checked function as we already
did an strlen() call anyway, so we won't have overflow issues, and
sprintf is more efficient

>
>        /* get the last update time, if it's there */
>        if((fp = fopen(file, "r")) == NULL) {
> @@ -90,6 +92,7 @@ int setlastupdate(const pmdb_t *db, time_t time)
>        FILE *fp;
>        char *file;
>        int ret = 0;
> +       int len = 0;
>
>        ALPM_LOG_FUNC;
>
> @@ -98,8 +101,9 @@ int setlastupdate(const pmdb_t *db, time_t time)
>        }
>
>        /* db->path + '.lastupdate' + NULL */
> -       MALLOC(file, strlen(db->path) + 12, RET_ERR(PM_ERR_MEMORY, ret));
> -       sprintf(file, "%s.lastupdate", db->path);
> +       len = strlen(db->path) + 12;
> +       CALLOC(file, len, sizeof(char), RET_ERR(PM_ERR_MEMORY, ret));
> +       snprintf(file, len, "%s.lastupdate", db->path);
Same as above.

>
>        if((fp = fopen(file, "w")) == NULL) {
>                free(file);
> @@ -123,9 +127,11 @@ int setlastupdate(const pmdb_t *db, time_t time)
>  int SYMEXPORT alpm_db_update(int force, pmdb_t *db)
>  {
>        alpm_list_t *lp;
> -       char path[PATH_MAX];
> +       char *dbfile, *dbfilepath;
>        time_t newmtime = 0, lastupdate = 0;
>        const char *dbpath;
> +       int len;
> +
>        int ret;
>
>        ALPM_LOG_FUNC;
> @@ -154,11 +160,15 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db)
>                }
>        }
>
> -       snprintf(path, PATH_MAX, "%s" DBEXT, db->treename);
> +       len = strlen(db->treename) + strlen(DBEXT) + 1;
> +       CALLOC(dbfile, len, sizeof(char), RET_ERR(PM_ERR_MEMORY, -1));
> +       snprintf(dbfile, len, "%s" DBEXT, db->treename);
Now this is legit; of course i would prefer the MALLOC/sprintf combo
as described above but it is nice to eliminate the PATH_MAX variable.

>        dbpath = alpm_option_get_dbpath();
>
> -       ret = _alpm_download_single_file(path, db->servers, dbpath,
> +       ret = _alpm_download_single_file(dbfile, db->servers, dbpath,
>                        lastupdate, &newmtime);
> +       free(dbfile);
>
>        if(ret == 1) {
>                /* mtimes match, do nothing */
> @@ -169,9 +179,6 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db)
>                _alpm_log(PM_LOG_DEBUG, "failed to sync db: %s\n", alpm_strerrorlast());
>                return(-1);
>        } else {
> -               /* form the path to the db location */
> -               snprintf(path, PATH_MAX, "%s%s" DBEXT, dbpath, db->treename);
> -
>                /* remove the old dir */
>                _alpm_log(PM_LOG_DEBUG, "flushing database %s\n", db->path);
>                for(lp = _alpm_db_get_pkgcache(db); lp; lp = lp->next) {
> @@ -186,11 +193,19 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db)
>                /* 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;
> +               CALLOC(dbfilepath, len, sizeof(char), RET_ERR(PM_ERR_MEMORY, -1));
> +               snprintf(dbfilepath, len, "%s%s" DBEXT, dbpath, db->treename);
> +
>                /* uncompress the sync database */
> -               if(_alpm_unpack(path, db->path, NULL)) {
> +               ret = _alpm_unpack(dbfilepath, db->path, NULL);
> +               if(ret) {
> +                       free(dbfilepath);
>                        RET_ERR(PM_ERR_SYSTEM, -1);
>                }
> -               unlink(path);
> +               unlink(dbfilepath);
> +               free(dbfilepath);
>
>                /* if we have a new mtime, set the DB last update value */
>                if(newmtime) {
> @@ -293,7 +308,7 @@ int _alpm_db_populate(pmdb_t *db)
>                        continue;
>                }
>                /* stat the entry, make sure it's a directory */
> -               snprintf(path, PATH_MAX, "%s/%s", db->path, name);
> +               snprintf(path, PATH_MAX, "%s%s", db->path, name);
>                if(stat(path, &sbuf) != 0 || !S_ISDIR(sbuf.st_mode)) {
>                        continue;
>                }
> @@ -329,12 +344,24 @@ int _alpm_db_populate(pmdb_t *db)
>        return(count);
>  }
>
> +
> +static char *get_pkgpath(pmdb_t *db, pmpkg_t *info)
Can you add some comments to this saying the return value must be
freed by the caller?
> +{
> +       int len;
> +       char *pkgpath;
> +       len = strlen(db->path) + strlen(info->name) + strlen(info->version) + 3;
> +       CALLOC(pkgpath, len, sizeof(char), RET_ERR(PM_ERR_MEMORY, NULL));
> +       snprintf(pkgpath, len, "%s%s-%s/", db->path, info->name, info->version);
MALLOC/sprintf.
> +       return(pkgpath);
> +}
> +
>  int _alpm_db_read(pmdb_t *db, pmpkg_t *info, pmdbinfrq_t inforeq)
>  {
>        FILE *fp = NULL;
>        struct stat buf;
>        char path[PATH_MAX];
>        char line[513];
> +       char *pkgpath = NULL;
>
>        ALPM_LOG_FUNC;
>
> @@ -367,17 +394,19 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info,
> pmdbinfrq_t inforeq)
>        /* clear out 'line', to be certain - and to make valgrind happy */
>        memset(line, 0, 513);
>
> -       snprintf(path, PATH_MAX, "%s/%s-%s", db->path, info->name, info->version);
> -       if(stat(path, &buf)) {
> +       pkgpath = get_pkgpath(db, info);
> +
> +       if(stat(pkgpath, &buf)) {
>                /* directory doesn't exist or can't be opened */
>                _alpm_log(PM_LOG_DEBUG, "cannot find '%s-%s' in db '%s'\n",
>                                info->name, info->version, db->treename);
> +               free(pkgpath);
>                return(-1);
>        }
>
>        /* DESC */
>        if(inforeq & INFRQ_DESC) {
> -               snprintf(path, PATH_MAX, "%s/%s-%s/desc", db->path, info->name,
> info->version);
> +               snprintf(path, PATH_MAX, "%sdesc", pkgpath);
>                if((fp = fopen(path, "r")) == NULL) {
>                        _alpm_log(PM_LOG_ERROR, _("could not open file %s: %s\n"), path,
> strerror(errno));
>                        goto error;
> @@ -521,7 +550,7 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info,
> pmdbinfrq_t inforeq)
>
>        /* FILES */
>        if(inforeq & INFRQ_FILES) {
> -               snprintf(path, PATH_MAX, "%s/%s-%s/files", db->path, info->name,
> info->version);
> +               snprintf(path, PATH_MAX, "%sfiles", pkgpath);
>                if((fp = fopen(path, "r")) == NULL) {
>                        _alpm_log(PM_LOG_ERROR, _("could not open file %s: %s\n"), path,
> strerror(errno));
>                        goto error;
> @@ -548,7 +577,7 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info,
> pmdbinfrq_t inforeq)
>
>        /* DEPENDS */
>        if(inforeq & INFRQ_DEPENDS) {
> -               snprintf(path, PATH_MAX, "%s/%s-%s/depends", db->path, info->name,
> info->version);
> +               snprintf(path, PATH_MAX, "%sdepends", pkgpath);
>                if((fp = fopen(path, "r")) == NULL) {
>                        _alpm_log(PM_LOG_ERROR, _("could not open file %s: %s\n"), path,
> strerror(errno));
>                        goto error;
> @@ -587,8 +616,7 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info,
> pmdbinfrq_t inforeq)
>
>        /* DELTAS */
>        if(inforeq & INFRQ_DELTAS) {
> -               snprintf(path, PATH_MAX, "%s/%s-%s/deltas", db->path,
> -                               info->name, info->version);
> +               snprintf(path, PATH_MAX, "%sdeltas", pkgpath);
>                if((fp = fopen(path, "r"))) {
>                        while(!feof(fp)) {
>                                fgets(line, 255, fp);
> @@ -606,7 +634,7 @@ int _alpm_db_read(pmdb_t *db, pmpkg_t *info,
> pmdbinfrq_t inforeq)
>
>        /* INSTALL */
>        if(inforeq & INFRQ_SCRIPTLET) {
> -               snprintf(path, PATH_MAX, "%s/%s-%s/install", db->path, info->name,
> info->version);
> +               snprintf(path, PATH_MAX, "%sinstall", pkgpath);
>                if(!stat(path, &buf)) {
>                        info->scriptlet = 1;
>                }
Ahh, much cleaner there in forming paths. But did you forget to free
pkgpath at the end of the function?

> @@ -632,6 +660,7 @@ int _alpm_db_write(pmdb_t *db, pmpkg_t *info,
> pmdbinfrq_t inforeq)
>        alpm_list_t *lp = NULL;
>        int retval = 0;
>        int local = 0;
> +       char *pkgpath = NULL;
>
>        ALPM_LOG_FUNC;
>
> @@ -639,9 +668,10 @@ int _alpm_db_write(pmdb_t *db, pmpkg_t *info,
> pmdbinfrq_t inforeq)
>                return(-1);
>        }
>
> -       snprintf(path, PATH_MAX, "%s/%s-%s", db->path, info->name, info->version);
> +       pkgpath = get_pkgpath(db, info);
> +
>        oldmask = umask(0000);
> -       mkdir(path, 0755);
> +       mkdir(pkgpath, 0755);
>        /* make sure we have a sane umask */
>        umask(0022);
>
> @@ -653,7 +683,7 @@ int _alpm_db_write(pmdb_t *db, pmpkg_t *info,
> pmdbinfrq_t inforeq)
>        if(inforeq & INFRQ_DESC) {
>                _alpm_log(PM_LOG_DEBUG, "writing %s-%s DESC information back to db\n",
>                                info->name, info->version);
> -               snprintf(path, PATH_MAX, "%s/%s-%s/desc", db->path, info->name,
> info->version);
> +               snprintf(path, PATH_MAX, "%sdesc", pkgpath);
>                if((fp = fopen(path, "w")) == NULL) {
>                        _alpm_log(PM_LOG_ERROR, _("could not open file %s: %s\n"), path,
> strerror(errno));
>                        retval = -1;
> @@ -741,7 +771,7 @@ int _alpm_db_write(pmdb_t *db, pmpkg_t *info,
> pmdbinfrq_t inforeq)
>        if(local && (inforeq & INFRQ_FILES)) {
>                _alpm_log(PM_LOG_DEBUG, "writing %s-%s FILES information back to db\n",
>                                info->name, info->version);
> -               snprintf(path, PATH_MAX, "%s/%s-%s/files", db->path, info->name,
> info->version);
> +               snprintf(path, PATH_MAX, "%sfiles", pkgpath);
>                if((fp = fopen(path, "w")) == NULL) {
>                        _alpm_log(PM_LOG_ERROR, _("could not open file %s: %s\n"), path,
> strerror(errno));
>                        retval = -1;
> @@ -769,7 +799,7 @@ int _alpm_db_write(pmdb_t *db, pmpkg_t *info,
> pmdbinfrq_t inforeq)
>        if(inforeq & INFRQ_DEPENDS) {
>                _alpm_log(PM_LOG_DEBUG, "writing %s-%s DEPENDS information back to db\n",
>                        info->name, info->version);
> -               snprintf(path, PATH_MAX, "%s/%s-%s/depends", db->path, info->name,
> info->version);
> +               snprintf(path, PATH_MAX, "%sdepends", pkgpath);
>                if((fp = fopen(path, "w")) == NULL) {
>                        _alpm_log(PM_LOG_ERROR, _("could not open file %s: %s\n"), path,
> strerror(errno));
>                        retval = -1;
> @@ -814,6 +844,7 @@ int _alpm_db_write(pmdb_t *db, pmpkg_t *info,
> pmdbinfrq_t inforeq)
>
>  cleanup:
>        umask(oldmask);
> +       free(pkgpath);
>
>        if(fp) {
>                fclose(fp);
> @@ -824,7 +855,8 @@ cleanup:
>
>  int _alpm_db_remove(pmdb_t *db, pmpkg_t *info)
>  {
> -       char path[PATH_MAX];
> +       int ret = 0;
> +       char *pkgpath = NULL;
>
>        ALPM_LOG_FUNC;
>
> @@ -832,12 +864,14 @@ int _alpm_db_remove(pmdb_t *db, pmpkg_t *info)
>                RET_ERR(PM_ERR_DB_NULL, -1);
>        }
>
> -       snprintf(path, PATH_MAX, "%s%s-%s", db->path, info->name, info->version);
> -       if(_alpm_rmrf(path) == -1) {
> -               return(-1);
> -       }
> +       pkgpath = get_pkgpath(db, info);
>
> -       return(0);
> +       ret = _alpm_rmrf(pkgpath);
> +       free(pkgpath);
> +       if(ret != 0) {
> +               ret = -1;
> +       }
> +       return(ret);
>  }
>
>  /* vim: set ts=2 sw=2 noet: */
> --
> 1.5.5.1