[pacman-dev] [PATCH 1/1] fix db filename handling

[Dave Reisner]

On Thu, Sep 05, 2013 at 01:12:10PM +1000, Allan McRae wrote:
> On 05/09/13 01:35, Christian Hesse wrote:
> > From: Christian Hesse <mail at eworm.de>
> > 
> > If the server redirects from ${repo}.db to ${repo}.db.tar.gz pacman gets
> > this wrong: It saves to new filename and fails when accessing
> > ${repo}.db.
> > 
> > This introduces a new field 'deny_rename' to payload. If set pacman
> > downloads to the filename specified in payload.remote_name.
> 
> Is there any case where we want to allow renaming of files?  We have
> expected names for both databases and package files (based on what is in
> the database).   So I guess this should be done by default?
> 

Yep, the only case where it makes sense to allow renaming is for -U
operations. We should probably rename the field (deny_rename seems
weird, since we *always* rename regardless). Maybe call it
"trust_remote_name" -- set this to true for alpm_fetch_pkgurl() and only
trust things like the content-disposition and redirects when
allow_remote_name is non-zero.

> > ---
> >  lib/libalpm/be_sync.c |  2 ++
> >  lib/libalpm/dload.c   | 36 ++++++++++++++++++++----------------
> >  lib/libalpm/dload.h   |  1 +
> >  3 files changed, 23 insertions(+), 16 deletions(-)
> > 
> > diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
> > index 123d953..622153d 100644
> > --- a/lib/libalpm/be_sync.c
> > +++ b/lib/libalpm/be_sync.c
> > @@ -223,6 +223,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
> >  		payload.handle = handle;
> >  		payload.force = force;
> >  		payload.unlink_on_fail = 1;
> > +		payload.deny_rename = 1;
> >  
> >  		ret = _alpm_download(&payload, syncpath, NULL, NULL);
> >  		_alpm_dload_payload_reset(&payload);
> > @@ -245,6 +246,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
> >  			snprintf(payload.fileurl, len, "%s/%s.db.sig", server, db->treename);
> >  			payload.handle = handle;
> >  			payload.force = 1;
> > +			payload.deny_rename = 1;
> >  			payload.errors_ok = (level & ALPM_SIG_DATABASE_OPTIONAL);
> >  
> >  			/* set hard upper limit of 16KiB */
> > diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> > index 8537b3d..381e548 100644
> > --- a/lib/libalpm/dload.c
> > +++ b/lib/libalpm/dload.c
> > @@ -547,23 +547,27 @@ static int curl_download_internal(struct dload_payload *payload,
> >  		goto cleanup;
> >  	}
> >  
> > -	if(payload->content_disp_name) {
> > -		/* content-disposition header has a better name for our file */
> > -		free(payload->destfile_name);
> > -		payload->destfile_name = get_fullpath(localpath, payload->content_disp_name, "");
> > +	if(payload->deny_rename) {
> > +		payload->destfile_name = get_fullpath(localpath, payload->remote_name, "");
> >  	} else {
> > -		const char *effective_filename = strrchr(effective_url, '/');
> > -		if(effective_filename && strlen(effective_filename) > 2) {
> > -			effective_filename++;
> > -
> > -			/* if destfile was never set, we wrote to a tempfile. even if destfile is
> > -			 * set, we may have followed some redirects and the effective url may
> > -			 * have a better suggestion as to what to name our file. in either case,
> > -			 * refactor destfile to this newly derived name. */
> > -			if(!payload->destfile_name || strcmp(effective_filename,
> > -						strrchr(payload->destfile_name, '/') + 1) != 0) {
> > -				free(payload->destfile_name);
> > -				payload->destfile_name = get_fullpath(localpath, effective_filename, "");
> > +		if(payload->content_disp_name) {
> > +			/* content-disposition header has a better name for our file */
> > +			free(payload->destfile_name);
> > +			payload->destfile_name = get_fullpath(localpath, payload->content_disp_name, "");
> > +		} else {
> > +			const char *effective_filename = strrchr(effective_url, '/');
> > +			if(effective_filename && strlen(effective_filename) > 2) {
> > +				effective_filename++;
> > +
> > +				/* if destfile was never set, we wrote to a tempfile. even if destfile is
> > +				 * set, we may have followed some redirects and the effective url may
> > +				 * have a better suggestion as to what to name our file. in either case,
> > +				 * refactor destfile to this newly derived name. */
> > +				if(!payload->destfile_name || strcmp(effective_filename,
> > +							strrchr(payload->destfile_name, '/') + 1) != 0) {
> > +					free(payload->destfile_name);
> > +					payload->destfile_name = get_fullpath(localpath, effective_filename, "");
> > +				}

Uggh. I wish I would have spent more time designing my downloader
rewrite... so many regrets...

> >  			}
> >  		}
> >  	}
> > diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
> > index 95bb91a..bd01d8b 100644
> > --- a/lib/libalpm/dload.h
> > +++ b/lib/libalpm/dload.h
> > @@ -38,6 +38,7 @@ struct dload_payload {
> >  	int allow_resume;
> >  	int errors_ok;
> >  	int unlink_on_fail;
> > +	int deny_rename;
> >  	alpm_list_t *servers;
> >  #ifdef HAVE_LIBCURL
> >  	CURLcode curlerr;       /* last error produced by curl */
> > 
> 
>