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 <[email protected]>
> >
> > 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 */
> >
>
>
