On Wed, Jan 8, 2020, 07:30 Allan McRae <al...@archlinux.org> wrote: > Sync databases (and signatures) are downloaded into a temporary directory. > On success, they are copied into the sync directory. >
Why not use a temporary file in the sync dir? That allows you to rename atomically instead of copy+unlink. My guess is it's less code, too. Currently, this achieves nothing but adding complexity, but it does open > up the possibility of validating sync databases on download before > replacing > the old version. > > Signed-off-by: Allan McRae <al...@archlinux.org> > --- > > This patch was long enough, and there is still quite a bit to do to > validate > the download sync databases before replacing the old ones, so best to stop > here. > > lib/libalpm/be_sync.c | 90 +++++++++++++++++++++++++++++++++---------- > 1 file changed, 69 insertions(+), 21 deletions(-) > > diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c > index f1caddd8..99bdba54 100644 > --- a/lib/libalpm/be_sync.c > +++ b/lib/libalpm/be_sync.c > @@ -173,8 +173,9 @@ valid: > */ > int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) > { > - char *syncpath; > - const char *dbext; > + char *syncpath, *sigpath = NULL; > + char *tmpdir = NULL, *tmppath = NULL, *tmpsig = NULL; > + const char *dbpath, *dbext; > alpm_list_t *i; > int updated = 0; > int ret = -1; > @@ -193,11 +194,22 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t > *db) > return 0; > } > > + /* attempt to grab a lock */ > + if(_alpm_handle_lock(handle)) { > + RET_ERR(handle, ALPM_ERR_HANDLE_LOCK, -1); > + } > + > syncpath = get_sync_dir(handle); > if(!syncpath) { > return -1; > } > > + /* create temporary directory to download databases into */ > + if(_alpm_mkdtemp(handle, &tmpdir) == 0) { > + free(syncpath); > + return -1; > + } > + > /* force update of invalid databases to fix potential mismatched > database/signature */ > if(db->status & DB_STATUS_INVALID) { > force = 1; > @@ -207,15 +219,41 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t > *db) > oldmask = umask(0022); > > siglevel = alpm_db_get_siglevel(db); > + dbext = handle->dbext; > > - /* attempt to grab a lock */ > - if(_alpm_handle_lock(handle)) { > - free(syncpath); > - umask(oldmask); > - RET_ERR(handle, ALPM_ERR_HANDLE_LOCK, -1); > + dbpath = _alpm_db_path(db); > + if(dbpath == NULL) { > + /* pm_errno set in _alpm_db_path() */ > + ret = -1; > + goto cleanup; > } > > - dbext = handle->dbext; > + if((sigpath = _alpm_sigpath(handle, _alpm_db_path(db))) == NULL) { > + /* pm_errno is set by _alpm_sigpath */ > + ret = -1; > + goto cleanup; > + } > + > + if(asprintf(&tmppath, "%s%s%s", tmpdir, db->treename, dbext) == > -1) { > + handle->pm_errno = ALPM_ERR_MEMORY; > + ret = -1; > + goto cleanup; > + } > + > + if(asprintf(&tmpsig, "%s%s%s.sig", tmpdir, db->treename, dbext) == > -1) { > + handle->pm_errno = ALPM_ERR_MEMORY; > + ret = -1; > + goto cleanup; > + } > + > + /* copy current db into tempdir to allow up-to-date db handling */ > + if(force == 0) { > + if(_alpm_copyfile_with_time(dbpath, tmppath) != 0) { > + handle->pm_errno = ALPM_ERR_SYSTEM; > + ret = -1; > + goto cleanup; > + }; > + } > > for(i = db->servers; i; i = i->next) { > const char *server = i->data, *final_db_url = NULL; > @@ -240,22 +278,11 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t > *db) > payload.force = force; > payload.unlink_on_fail = 1; > > - ret = _alpm_download(&payload, syncpath, NULL, > &final_db_url); > + ret = _alpm_download(&payload, tmpdir, NULL, > &final_db_url); > _alpm_dload_payload_reset(&payload); > updated = (updated || ret == 0); > > if(ret != -1 && updated && (siglevel & ALPM_SIG_DATABASE)) > { > - /* an existing sig file is no good at this point */ > - char *sigpath = _alpm_sigpath(handle, > _alpm_db_path(db)); > - if(!sigpath) { > - /* pm_errno is set by _alpm_sigpath */ > - ret = -1; > - goto cleanup; > - } > - unlink(sigpath); > - free(sigpath); > - > - > /* check if the final URL from internal downloader > looks reasonable */ > if(final_db_url != NULL) { > if(strlen(final_db_url) < 3 > @@ -295,7 +322,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) > /* set hard upper limit of 16KiB */ > payload.max_size = 16 * 1024; > > - sig_ret = _alpm_download(&payload, syncpath, NULL, > NULL); > + sig_ret = _alpm_download(&payload, tmpdir, NULL, > NULL); > /* errors_ok suppresses error messages, but not > the return code */ > sig_ret = payload.errors_ok ? 0 : sig_ret; > _alpm_dload_payload_reset(&payload); > @@ -306,6 +333,15 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) > } > } > > + /* copy the downloaded database into the sync directory */ > + unlink(dbpath); > + _alpm_copyfile_with_time(tmppath, dbpath); > + > + unlink(sigpath); > + if(_alpm_access(handle, NULL, sigpath, R_OK) == 0) { > + _alpm_copyfile_with_time(tmpsig, sigpath); > + } > + > cleanup: > if(updated) { > /* Cache needs to be rebuilt */ > @@ -332,8 +368,20 @@ cleanup: > handle->pm_errno = ALPM_ERR_OK; > } > > + /* clean-up temporary directory */ > + unlink(tmppath); > + unlink(tmpsig); > + if(rmdir(tmpdir)) { > + _alpm_log(handle, ALPM_LOG_WARNING, > + _("could not remove tmpdir %s\n"), tmpdir); > + } > + free(tmpdir); > + free(tmppath); > + free(tmpsig); > + > _alpm_handle_unlock(handle); > free(syncpath); > + free(sigpath); > umask(oldmask); > return ret; > } > -- > 2.24.1 >