On Fri, Jul 1, 2011 at 7:59 AM, Dave Reisner <d@falconindy.com> wrote:

From: Dave Reisner <d@falconindy.com>

URLs might end with a slash and follow redirects, or could be a generated by a script such as /getpkg.php?id=12345. In both cases, we may have a better filename that we can write to, taken from either content-disposition header, or the effective URL.

Specific to the first case, we write to a temporary file of the format 'alpmtmp.XXXXXX', where XXXXXX is randomized by mkstemp(3). Since this is a randomly generated file, we cannot support resuming and the file is unlinked in the event of an interrupt.

We also run into the possibility of changing out the filename from under alpm on a -U operation, so callers of _alpm_download can optionally pass a pointer to a *char to be filled in by curl_download_internal with the actual filename we wrote to. Any sync operation will pass a NULL pointer here, as we rely on specific names for packages from a mirror.

Fixes FS#22645.

Signed-off-by: Dave Reisner <d@falconindy.com> ---  lib/libalpm/be_sync.c |    4 +-  lib/libalpm/dload.c   |  129 +++++++++++++++++++++++++++++++++++++++++--------  lib/libalpm/dload.h   |    3 +-  lib/libalpm/sync.c    |    2 +-  4 files changed, 114 insertions(+), 24 deletions(-)

diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index ce418d5..16351f8 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -184,7 +184,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)                CALLOC(fileurl, len, sizeof(char), RET_ERR(handle, PM_ERR_MEMORY, -1));                snprintf(fileurl, len, "%s/%s.db", server, db->treename);

-               ret = _alpm_download(handle, fileurl, syncpath, force, 0, 0); +               ret = _alpm_download(handle, fileurl, syncpath, NULL, force, 0, 0);

               if(ret == 0 && (check_sig == PM_PGP_VERIFY_ALWAYS ||                                        check_sig == PM_PGP_VERIFY_OPTIONAL)) { @@ -201,7 +201,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)                        /* if we downloaded a DB, we want the .sig from the same server */                        snprintf(fileurl, len, "%s/%s.db.sig", server, db->treename);

-                       sig_ret = _alpm_download(handle, fileurl, syncpath, 1, 0, errors_ok); +                       sig_ret = _alpm_download(handle, fileurl, syncpath, NULL, 1, 0, errors_ok);                        /* errors_ok suppresses error messages, but not the return code */                        sig_ret = errors_ok ? 0 : sig_ret;                } diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index fd83aac..eefa285 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -148,16 +148,46 @@ static int utimes_long(const char *path, long seconds)        return 0;  }

+static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user) +{ +       size_t realsize = size * nmemb; +       const char *fptr, *endptr = NULL; +       const char * const cd_header = "Content-Disposition:"; +       const char * const fn_key = "filename="; +       struct fileinfo **dlfile = (struct fileinfo**)user; + +       if(strncasecmp(cd_header, ptr, strlen(cd_header)) == 0) { Locale issues come into play? This string has an 'i'. If cURL normalizes headers one way or the other we should probably do a direct compare, but the strncasemp() isn't explicit regarding locale or anything, so it may just map lower/upper ASCII to the opposite ASCII and that is it.

+               if((fptr = strstr(ptr, fn_key))) { +                       fptr += strlen(fn_key); + +                       /* find the end of the field, which is either a semi-colon, or the end of +                        * the data. As per curl_easy_setopt(3), we cannot count on headers being +                        * null terminated, so we look for the closing \r\n */ +                       endptr = fptr + strcspn(fptr, ";\r\n") - 1; + +                       /* remove quotes */ +                       if(*fptr == '"' && *endptr == '"') { +                               fptr++; +                               endptr--; +                       } + +                       STRNDUP((*dlfile)->cd_filename, fptr, endptr - fptr + 1, +                                       RET_ERR((*dlfile)->handle, PM_ERR_MEMORY, realsize)); +               } +       }

-static int curl_download_internal(alpm_handle_t *handle, -               const char *url, const char *localpath, -               int force, int allow_resume, int errors_ok) +       return realsize; +} + +static int curl_download_internal(alpm_handle_t *handle, const char *url, +               const char *localpath, char **final_file, int force, int allow_resume, +               int errors_ok) I believe this hideous function signature gets addressed later, so I won't worry about it now.

 { -       int ret = -1; +       int ret = -1, should_unlink = 0;        FILE *localf = NULL;        const char *useragent;        const char *open_mode = "wb"; -       char *destfile, *tempfile; +       char *destfile = NULL, *tempfile = NULL, *effective_url;        /* RFC1123 states applications should support this length */        char hostname[256];        char error_buffer[CURL_ERROR_SIZE]; @@ -170,15 +200,39 @@ static int curl_download_internal(alpm_handle_t *handle,        dlfile.handle = handle;        dlfile.initial_size = 0.0;        dlfile.filename = get_filename(url); +       dlfile.cd_filename = NULL;        if(!dlfile.filename || curl_gethost(url, hostname) != 0) {                _alpm_log(handle, PM_LOG_ERROR, _("url '%s' is invalid\n"), url);                RET_ERR(handle, PM_ERR_SERVER_BAD_URL, -1);        }

-       destfile = get_fullpath(localpath, dlfile.filename, ""); -       tempfile = get_fullpath(localpath, dlfile.filename, ".part"); -       if(!destfile || !tempfile) { -               goto cleanup; +       if(strlen(dlfile.filename) > 0 && strcmp(dlfile.filename, ".sig") != 0) { +               destfile = get_fullpath(localpath, dlfile.filename, ""); +               tempfile = get_fullpath(localpath, dlfile.filename, ".part"); +               if(!destfile || !tempfile) { +                       goto cleanup; +               } +       } else { +               /* URL isn't to a file and ended with a slash */ +               int fd; +               char randpath[PATH_MAX]; + +               /* we can't support resuming this kind of download, so a partial transfer +                * will be destroyed */ +               should_unlink = 1; + +               /* create a random filename, which is opened with O_EXCL */ +               snprintf(randpath, PATH_MAX, "%salpmtmp.XXXXXX", localpath); +               if((fd = mkstemp(randpath)) == -1 || !(localf = fdopen(fd, open_mode))) { +                       unlink(randpath); +                       close(fd); +                       _alpm_log(handle, PM_LOG_ERROR, +                                       _("failed to create temporary file for download\n")); +                       goto cleanup; +               } +               /* localf now points to our alpmtmp.XXXXXX */ +               STRDUP(tempfile, randpath, RET_ERR(handle, PM_ERR_MEMORY, -1)); +               dlfile.filename = strrchr(randpath, '/') + 1;        }

       error_buffer[0] = '\0'; @@ -197,6 +251,8 @@ static int curl_download_internal(alpm_handle_t *handle,        curl_easy_setopt(handle->curl, CURLOPT_PROGRESSDATA, (void *)&dlfile);        curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_LIMIT, 1024L);        curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_TIME, 10L); +       curl_easy_setopt(handle->curl, CURLOPT_HEADERFUNCTION, parse_headers); +       curl_easy_setopt(handle->curl, CURLOPT_WRITEHEADER, &dlfile);

       useragent = getenv("HTTP_USER_AGENT");        if(useragent != NULL) { @@ -215,9 +271,11 @@ static int curl_download_internal(alpm_handle_t *handle,                dlfile.initial_size = (double)st.st_size;        }

-       localf = fopen(tempfile, open_mode);        if(localf == NULL) { -               goto cleanup; +               localf = fopen(tempfile, open_mode); +               if(localf == NULL) { +                       goto cleanup; +               }        }

       curl_easy_setopt(handle->curl, CURLOPT_WRITEDATA, localf); @@ -264,6 +322,7 @@ static int curl_download_internal(alpm_handle_t *handle,        curl_easy_getinfo(handle->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &remote_size);        curl_easy_getinfo(handle->curl, CURLINFO_SIZE_DOWNLOAD, &bytes_dl);        curl_easy_getinfo(handle->curl, CURLINFO_CONDITION_UNMET, &timecond); +       curl_easy_getinfo(handle->curl, CURLINFO_EFFECTIVE_URL, &effective_url);

       /* time condition was met and we didn't download anything. we need to         * clean up the 0 byte .part file that's left behind. */ @@ -284,6 +343,26 @@ static int curl_download_internal(alpm_handle_t *handle,                goto cleanup;        }

+       if(dlfile.cd_filename) { +               /* content-disposition header has a better name for our file */ +               free(destfile); +               destfile = get_fullpath(localpath, dlfile.cd_filename, ""); +       } else { +               const char *effective_filename = strrchr(effective_url, '/'); +               if(effective_filename) { +                       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(!destfile || strcmp(effective_filename, strrchr(destfile, '/') + 1) != 0) { Any chance of strrchr returning NULL here?

+                               free(destfile); +                               destfile = get_fullpath(localpath, effective_filename, ""); +                       } +               } +       } +        ret = 0;

 cleanup: @@ -294,10 +373,18 @@ cleanup:

       if(ret == 0) {                rename(tempfile, destfile); +               if(final_file) { +                       *final_file = strdup(strrchr(destfile, '/') + 1); Same concern as above with strrchr(). STRDUP would be nice too.

+               } +       } + +       if(dload_interrupted && should_unlink) { +               unlink(tempfile);        }

       FREE(tempfile);        FREE(destfile); +       FREE(dlfile.cd_filename);

       /* restore the old signal handlers */        sigaction(SIGINT, &sig_int[OLD], NULL); @@ -316,18 +403,19 @@ cleanup:  * @param handle the context handle  * @param url the file's URL  * @param localpath the directory to save the file in + * @param final_file the real name of the downloaded file (may be NULL)  * @param force force download even if there is an up-to-date local copy  * @param allow_resume allow a partial download to be resumed  * @param errors_ok do not log errors (but still return them)  * @return 0 on success, -1 on error (pm_errno is set accordingly if errors_ok == 0)  */  int _alpm_download(alpm_handle_t *handle, const char *url, const char *localpath, -               int force, int allow_resume, int errors_ok) +               char **final_file, int force, int allow_resume, int errors_ok)  {        if(handle->fetchcb == NULL) {  #ifdef HAVE_LIBCURL -               return curl_download_internal(handle, url, localpath, -                               force, allow_resume, errors_ok); +               return curl_download_internal(handle, url, localpath, final_file, force, +                               allow_resume, errors_ok);  #else                RET_ERR(handle, PM_ERR_EXTERNAL_DOWNLOAD, -1);  #endif @@ -344,18 +432,17 @@ int _alpm_download(alpm_handle_t *handle, const char *url, const char *localpath  char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url)  {        char *filepath; -       const char *filename, *cachedir; +       const char *cachedir; +       char *final_file = NULL;        int ret;

       CHECK_HANDLE(handle, return NULL);

-       filename = get_filename(url); -        /* find a valid cache dir to download to */        cachedir = _alpm_filecache_setup(handle);

       /* download the file */ -       ret = _alpm_download(handle, url, cachedir, 0, 1, 0); +       ret = _alpm_download(handle, url, cachedir, &final_file, 0, 1, 0);        if(ret == -1) {                _alpm_log(handle, PM_LOG_WARNING, _("failed to download %s\n"), url);                return NULL; @@ -373,7 +460,7 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url)                CALLOC(sig_url, len, sizeof(char), RET_ERR(handle, PM_ERR_MEMORY, NULL));                snprintf(sig_url, len, "%s.sig", url);

-               ret = _alpm_download(handle, sig_url, cachedir, 1, 0, errors_ok); +               ret = _alpm_download(handle, sig_url, cachedir, &final_file, 1, 0, errors_ok);                if(ret == -1 && !errors_ok) {                        _alpm_log(handle, PM_LOG_WARNING, _("failed to download %s\n"), sig_url);                        /* Warn now, but don't return NULL. We will fail later during package @@ -385,7 +472,9 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url)        }

       /* we should be able to find the file the second time around */ -       filepath = _alpm_filecache_find(handle, filename); +       filepath = _alpm_filecache_find(handle, final_file); +       FREE(final_file); +        return filepath;  }

diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h index 0cdd900..351b2b3 100644 --- a/lib/libalpm/dload.h +++ b/lib/libalpm/dload.h @@ -29,11 +29,12 @@  struct fileinfo {        alpm_handle_t *handle;        const char *filename; +       char *cd_filename;        double initial_size;  };

 int _alpm_download(alpm_handle_t *handle, const char *url, const char *localpath, -               int force, int allow_resume, int errors_ok); +               char **final_file, int force, int allow_resume, int errors_ok);

 #endif /* _ALPM_DLOAD_H */

diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 8f03840..8df8a17 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -797,7 +797,7 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas)                                        CALLOC(fileurl, len, sizeof(char), RET_ERR(handle, PM_ERR_MEMORY, -1));                                        snprintf(fileurl, len, "%s/%s", server_url, filename);

-                                       ret = _alpm_download(handle, fileurl, cachedir, 0, 1, 0); +                                       ret = _alpm_download(handle, fileurl, cachedir, NULL, 0, 1, 0);                                        FREE(fileurl);                                        if(ret != -1) {                                                break; -- 1.7.6

On Sun, Jul 03, 2011 at 02:13:30PM -0500, Dan McGee wrote:

On Fri, Jul 1, 2011 at 7:59 AM, Dave Reisner <d@falconindy.com> wrote:

...

From: Dave Reisner <d@falconindy.com>

This means creating a new struct which can pass more descriptive data from the back end sync functions to the downloader. In particular, we're interested in the download size read from the sync DB. When the remote server reports a size larger than this (via a content-length header), abort the transfer.

In cases where the size is unknown, we set a hard upper limit of:

* 25MiB for a sync DB * 16KiB for a signature

For reference, 25mb is more than twice the size of all of the current binary repos (with files) combined, and 16k is a truly gargantuan signature. MiB/KiB here to not confuse people, please. (Or MB/KB, but definitely not lowercase). Same with any usages in the code comments.

Fixed.

...

Signed-off-by: Dave Reisner <dreisner@archlinux.org> ---  lib/libalpm/be_sync.c |   22 +++++++++++++------  lib/libalpm/dload.c   |   55 +++++++++++++++++++++++++++++++++---------------  lib/libalpm/dload.h   |   13 +++++++++-  lib/libalpm/sync.c    |   30 +++++++++++++++++---------  4 files changed, 84 insertions(+), 36 deletions(-)

diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 16351f8..65420cf 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -175,16 +175,21 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)

       for(i = db->servers; i; i = i->next) {                const char *server = i->data; -               char *fileurl; +               struct dload_payload *payload;                size_t len;                int sig_ret = 0;

+               CALLOC(payload, 1, sizeof(*payload), RET_ERR(handle, PM_ERR_MEMORY, -1));

Any reason not to just allocate it on the stack and not use a pointer? You will want to memset(&payload, 0, sizeof(payload); then.

Edit: after reading the rest of the patch, we need heap allocated payloads elsewhere, so you can probably ignore me here.

Yep, I considered this, but opted for heap allocations throughout since it wasn't always possible to stack allocate.

...

+ +               /* set hard upper limit of 25MiB */ +               payload->max_size = 25 * 1024 * 1024; +                /* print server + filename into a buffer (leave space for .sig) */                len = strlen(server) + strlen(db->treename) + 9; -               CALLOC(fileurl, len, sizeof(char), RET_ERR(handle, PM_ERR_MEMORY, -1)); -               snprintf(fileurl, len, "%s/%s.db", server, db->treename); +               CALLOC(payload->fileurl, len, sizeof(char), RET_ERR(handle, PM_ERR_MEMORY, -1)); +               snprintf(payload->fileurl, len, "%s/%s.db", server, db->treename);

-               ret = _alpm_download(handle, fileurl, syncpath, NULL, force, 0, 0); +               ret = _alpm_download(handle, payload, syncpath, NULL, force, 0, 0);

               if(ret == 0 && (check_sig == PM_PGP_VERIFY_ALWAYS ||                                        check_sig == PM_PGP_VERIFY_OPTIONAL)) { @@ -199,14 +204,17 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)

                       int errors_ok = (check_sig == PM_PGP_VERIFY_OPTIONAL);                        /* if we downloaded a DB, we want the .sig from the same server */ -                       snprintf(fileurl, len, "%s/%s.db.sig", server, db->treename); +                       snprintf(payload->fileurl, len, "%s/%s.db.sig", server, db->treename); + +                       /* set hard upper limited of 16KiB */ "limit"

Fixed.

...

+                       payload->max_size = 16 * 1024;

-                       sig_ret = _alpm_download(handle, fileurl, syncpath, NULL, 1, 0, errors_ok); +                       sig_ret = _alpm_download(handle, payload, syncpath, NULL, 1, 0, errors_ok);                        /* errors_ok suppresses error messages, but not the return code */                        sig_ret = errors_ok ? 0 : sig_ret;                }

-               FREE(fileurl); +               _alpm_dload_payload_free(payload); If you do stack-based, this will have to change to not free &payload itself, but would still be useful as a helper to free all the attached strings.

Ignoring this.

...

               if(ret != -1 && sig_ret != -1) {                        break;                } diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index eefa285..1758744 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -179,7 +179,7 @@ static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user)        return realsize;  }

-static int curl_download_internal(alpm_handle_t *handle, const char *url, +static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *payload,                const char *localpath, char **final_file, int force, int allow_resume,                int errors_ok)  { @@ -199,10 +199,10 @@ static int curl_download_internal(alpm_handle_t *handle, const char *url,

       dlfile.handle = handle;        dlfile.initial_size = 0.0; -       dlfile.filename = get_filename(url); +       dlfile.filename = get_filename(payload->fileurl);        dlfile.cd_filename = NULL; -       if(!dlfile.filename || curl_gethost(url, hostname) != 0) { -               _alpm_log(handle, PM_LOG_ERROR, _("url '%s' is invalid\n"), url); +       if(!dlfile.filename || curl_gethost(payload->fileurl, hostname) != 0) { +               _alpm_log(handle, PM_LOG_ERROR, _("url '%s' is invalid\n"), payload->fileurl);                RET_ERR(handle, PM_ERR_SERVER_BAD_URL, -1);        }

@@ -240,7 +240,7 @@ static int curl_download_internal(alpm_handle_t *handle, const char *url,        /* the curl_easy handle is initialized with the alpm handle, so we only need         * to reset the curl handle set parameters for each time it's used. */        curl_easy_reset(handle->curl); -       curl_easy_setopt(handle->curl, CURLOPT_URL, url); +       curl_easy_setopt(handle->curl, CURLOPT_URL, payload->fileurl);        curl_easy_setopt(handle->curl, CURLOPT_FAILONERROR, 1L);        curl_easy_setopt(handle->curl, CURLOPT_ERRORBUFFER, error_buffer);        curl_easy_setopt(handle->curl, CURLOPT_CONNECTTIMEOUT, 10L); @@ -254,6 +254,10 @@ static int curl_download_internal(alpm_handle_t *handle, const char *url,        curl_easy_setopt(handle->curl, CURLOPT_HEADERFUNCTION, parse_headers);        curl_easy_setopt(handle->curl, CURLOPT_WRITEHEADER, &dlfile);

+       if(payload->max_size) { +               curl_easy_setopt(handle->curl, CURLOPT_MAXFILESIZE, payload->max_size);

So I hate to rain on the parade here, but: CURLOPT_MAXFILESIZE Pass a long as parameter. This allows you to specify the maximum size (in bytes) of a file to download. If the file requested is larger than this value, the transfer will not start and CURLE_FILESIZE_EXCEEDED will be returned. The file size is not always known prior to download, and for such files this option has no effect even if the file transfer ends up being larger than this given limit. This concerns both FTP and HTTP transfers.

Which seems to indicate it is rather easy to bypass said restrictions if you simply put up a server that forgets the Content-Length header. Do we need to do something in the progress callback as well to check up on how much we've downloaded?

In theory, yeah -- we should probably add this. That said, I did a survey of all our current known mirrors, and failed to find any which did _not_ report a content-length header.

...

+       } +        useragent = getenv("HTTP_USER_AGENT");        if(useragent != NULL) {                curl_easy_setopt(handle->curl, CURLOPT_USERAGENT, useragent); @@ -409,18 +413,19 @@ cleanup:  * @param errors_ok do not log errors (but still return them)  * @return 0 on success, -1 on error (pm_errno is set accordingly if errors_ok == 0)  */ -int _alpm_download(alpm_handle_t *handle, const char *url, const char *localpath, -               char **final_file, int force, int allow_resume, int errors_ok) +int _alpm_download(alpm_handle_t *handle, struct dload_payload *payload, +               const char *localpath, char **final_file, int force, int allow_resume, +               int errors_ok)  {        if(handle->fetchcb == NULL) {  #ifdef HAVE_LIBCURL -               return curl_download_internal(handle, url, localpath, final_file, force, +               return curl_download_internal(handle, payload, localpath, final_file, force,                                allow_resume, errors_ok);  #else                RET_ERR(handle, PM_ERR_EXTERNAL_DOWNLOAD, -1);  #endif        } else { -               int ret = handle->fetchcb(url, localpath, force); +               int ret = handle->fetchcb(payload->fileurl, localpath, force);                if(ret == -1 && !errors_ok) {                        RET_ERR(handle, PM_ERR_EXTERNAL_DOWNLOAD, -1);                } @@ -434,6 +439,7 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url)        char *filepath;        const char *cachedir;        char *final_file = NULL; +       struct dload_payload *payload;        int ret;

       CHECK_HANDLE(handle, return NULL); @@ -441,8 +447,11 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url)        /* find a valid cache dir to download to */        cachedir = _alpm_filecache_setup(handle);

+       CALLOC(payload, 1, sizeof(*payload), RET_ERR(handle, PM_ERR_MEMORY, NULL)); +       payload->filename = strdup(url); +        /* download the file */ -       ret = _alpm_download(handle, url, cachedir, &final_file, 0, 1, 0); +       ret = _alpm_download(handle, payload, cachedir, &final_file, 0, 1, 0);        if(ret == -1) {                _alpm_log(handle, PM_LOG_WARNING, _("failed to download %s\n"), url);                return NULL; @@ -452,30 +461,42 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url)        /* attempt to download the signature */        if(ret == 0 && (handle->sigverify == PM_PGP_VERIFY_ALWAYS ||                                handle->sigverify == PM_PGP_VERIFY_OPTIONAL)) { -               char *sig_url;                size_t len;                int errors_ok = (handle->sigverify == PM_PGP_VERIFY_OPTIONAL); +               struct dload_payload *spayload; Is the load getting spayed? sig_payload would be a bit more readable. It also looks like you never set the 16 KiB limit here.

Just trying to make Bob Barker happy.

...

+               CALLOC(spayload, 1, sizeof(*spayload), RET_ERR(handle, PM_ERR_MEMORY, NULL));                len = strlen(url) + 5; -               CALLOC(sig_url, len, sizeof(char), RET_ERR(handle, PM_ERR_MEMORY, NULL)); -               snprintf(sig_url, len, "%s.sig", url); +               CALLOC(spayload->fileurl, len, sizeof(char), RET_ERR(handle, PM_ERR_MEMORY, NULL)); +               snprintf(spayload->fileurl, len, "%s.sig", url);

-               ret = _alpm_download(handle, sig_url, cachedir, &final_file, 1, 0, errors_ok); +               ret = _alpm_download(handle, spayload, cachedir, &final_file, 1, 0, errors_ok);                if(ret == -1 && !errors_ok) { -                       _alpm_log(handle, PM_LOG_WARNING, _("failed to download %s\n"), sig_url); +                       _alpm_log(handle, PM_LOG_WARNING, _("failed to download %s\n"), spayload->fileurl);                        /* Warn now, but don't return NULL. We will fail later during package                         * load time. */                } else if(ret == 0) { -                       _alpm_log(handle, PM_LOG_DEBUG, "successfully downloaded %s\n", sig_url); +                       _alpm_log(handle, PM_LOG_DEBUG, "successfully downloaded %s\n", spayload->fileurl);                } -               FREE(sig_url); +               _alpm_dload_payload_free(spayload);        }

       /* we should be able to find the file the second time around */        filepath = _alpm_filecache_find(handle, final_file);        FREE(final_file); +       _alpm_dload_payload_free(payload);

       return filepath;  }

+void _alpm_dload_payload_free(void *payload) { +       struct dload_payload *load = (struct dload_payload*)payload; + +       ASSERT(load, return); + +       FREE(load->filename); +       FREE(load->fileurl); +       FREE(load); +} +  /* vim: set ts=2 sw=2 noet: */ diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h index 351b2b3..8aaed0c 100644 --- a/lib/libalpm/dload.h +++ b/lib/libalpm/dload.h @@ -33,8 +33,17 @@ struct fileinfo {        double initial_size;  };

-int _alpm_download(alpm_handle_t *handle, const char *url, const char *localpath, -               char **final_file, int force, int allow_resume, int errors_ok); +struct dload_payload { +       char *filename; +       char *fileurl; +       long max_size; +}; + +void _alpm_dload_payload_free(void *payload); + +int _alpm_download(alpm_handle_t *handle, struct dload_payload *payload, +               const char *localpath, char **final_file, int force, int allow_resume, +               int errors_ok);

 #endif /* _ALPM_DLOAD_H */

diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 8df8a17..4277283 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -757,10 +757,12 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas)                        alpm_pkg_t *spkg = j->data;

                       if(spkg->origin != PKG_FROM_FILE && current == spkg->origin_data.db) { -                               const char *fname = NULL; +                               struct dload_payload *payload; + +                               CALLOC(payload, 1, sizeof(*payload), RET_ERR(handle, PM_ERR_MEMORY, -1)); +                               STRDUP(payload->filename, spkg->filename, RET_ERR(handle, PM_ERR_MEMORY, -1)); +                               payload->max_size = spkg->download_size;

This is actually wrong. Take a look at where and when we set this field (compute_download_size). You never want a possible delta chain total size, so you really want to use alpm_pkg_get_size() in all cases here.

Fixed. Good to know.

Sidenote: I also have a mental TODO in the future of allowing compute_download_size() to take partial downloads into account, so if you say had half of libreoffice downloaded, it would report the proper size at the prompt.

Seems reasonable.

...

-                               fname = alpm_pkg_get_filename(spkg); -                               ASSERT(fname != NULL, RET_ERR(handle, PM_ERR_PKG_INVALID_NAME, -1));                                alpm_list_t *delta_path = spkg->delta_path;                                if(delta_path) {                                        /* using deltas */ @@ -768,14 +770,20 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas)                                        for(dlts = delta_path; dlts; dlts = dlts->next) {                                                alpm_delta_t *delta = dlts->data;                                                if(delta->download_size != 0) { -                                                       files = alpm_list_add(files, strdup(delta->delta)); +                                                       struct dload_payload *dpayload; + +                                                       CALLOC(dpayload, 1, sizeof(*payload), RET_ERR(handle, PM_ERR_MEMORY, -1)); +                                                       STRDUP(dpayload->filename, delta->delta, RET_ERR(handle, PM_ERR_MEMORY, -1)); +                                                       dpayload->max_size = delta->download_size; + +                                                       files = alpm_list_add(files, dpayload);                                                }                                                /* keep a list of all the delta files for md5sums */                                                *deltas = alpm_list_add(*deltas, delta);                                        }

                               } else if(spkg->download_size != 0) { -                                       files = alpm_list_add(files, strdup(fname)); +                                       files = alpm_list_add(files, payload);                                }

                       } @@ -784,7 +792,7 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas)                if(files) {                        EVENT(handle->trans, PM_TRANS_EVT_RETRIEVE_START, current->treename, NULL);                        for(j = files; j; j = j->next) { -                               const char *filename = j->data; +                               struct dload_payload *payload = j->data;                                alpm_list_t *server;                                int ret = -1;                                for(server = current->servers; server; server = server->next) { @@ -793,11 +801,11 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas)                                        size_t len;

                                       /* print server + filename into a buffer */ -                                       len = strlen(server_url) + strlen(filename) + 2; -                                       CALLOC(fileurl, len, sizeof(char), RET_ERR(handle, PM_ERR_MEMORY, -1)); -                                       snprintf(fileurl, len, "%s/%s", server_url, filename); +                                       len = strlen(server_url) + strlen(payload->filename) + 2; +                                       CALLOC(payload->fileurl, len, sizeof(char), RET_ERR(handle, PM_ERR_MEMORY, -1)); +                                       snprintf(payload->fileurl, len, "%s/%s", server_url, payload->filename);

-                                       ret = _alpm_download(handle, fileurl, cachedir, NULL, 0, 1, 0); +                                       ret = _alpm_download(handle, payload, cachedir, NULL, 0, 1, 0);                                        FREE(fileurl);                                        if(ret != -1) {                                                break; @@ -809,6 +817,8 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas)                        }

                       FREELIST(files); +                       alpm_list_free_inner(files, _alpm_dload_payload_free);

This is going to segfault, you left the FREELIST() call in; you want to remove that completely.

I'm surprised I haven't seen a double free yet because of this.

...

+                       alpm_list_free(files);                        if(errors) {                                _alpm_log(handle, PM_LOG_WARNING, _("failed to retrieve some files from %s\n"),                                                current->treename); -- 1.7.6

On Fri, Jul 01, 2011 at 08:59:25AM -0400, Dave Reisner wrote:

Restore some sanity to the number of arguments passed to _alpm_download and curl_download_internal.

Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/be_sync.c | 12 +++-- lib/libalpm/dload.c | 118 ++++++++++++++++++++++++------------------------ lib/libalpm/dload.h | 9 +++- lib/libalpm/sync.c | 4 +- 4 files changed, 76 insertions(+), 67 deletions(-)

diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 65420cf..9a3046a 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -188,8 +188,10 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) len = strlen(server) + strlen(db->treename) + 9; CALLOC(payload->fileurl, len, sizeof(char), RET_ERR(handle, PM_ERR_MEMORY, -1)); snprintf(payload->fileurl, len, "%s/%s.db", server, db->treename); + payload->handle = handle; + payload->force = force;

- ret = _alpm_download(handle, payload, syncpath, NULL, force, 0, 0); + ret = _alpm_download(payload, syncpath, NULL);

if(ret == 0 && (check_sig == PM_PGP_VERIFY_ALWAYS || check_sig == PM_PGP_VERIFY_OPTIONAL)) { @@ -202,16 +204,18 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) unlink(sigpath); free(sigpath);

- int errors_ok = (check_sig == PM_PGP_VERIFY_OPTIONAL); /* if we downloaded a DB, we want the .sig from the same server */ snprintf(payload->fileurl, len, "%s/%s.db.sig", server, db->treename); + payload->handle = handle; + payload->force = 1; + payload->errors_ok = (check_sig == PM_PGP_VERIFY_OPTIONAL);

/* set hard upper limited of 16KiB */ payload->max_size = 16 * 1024;

- sig_ret = _alpm_download(handle, payload, syncpath, NULL, 1, 0, errors_ok); + sig_ret = _alpm_download(payload, syncpath, NULL); /* errors_ok suppresses error messages, but not the return code */ - sig_ret = errors_ok ? 0 : sig_ret; + sig_ret = payload->errors_ok ? 0 : sig_ret; }

_alpm_dload_payload_free(payload); diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 1758744..bac9356 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -179,9 +179,8 @@ static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user) return realsize; }

-static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *payload, - const char *localpath, char **final_file, int force, int allow_resume, - int errors_ok) +static int curl_download_internal(struct dload_payload *payload, + const char *localpath, char **final_file) { int ret = -1, should_unlink = 0; FILE *localf = NULL; @@ -197,13 +196,13 @@ static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *p struct sigaction sig_pipe[2], sig_int[2]; struct fileinfo dlfile;

- dlfile.handle = handle; + dlfile.handle = payload->handle; dlfile.initial_size = 0.0; dlfile.filename = get_filename(payload->fileurl); dlfile.cd_filename = NULL; if(!dlfile.filename || curl_gethost(payload->fileurl, hostname) != 0) { - _alpm_log(handle, PM_LOG_ERROR, _("url '%s' is invalid\n"), payload->fileurl); - RET_ERR(handle, PM_ERR_SERVER_BAD_URL, -1); + _alpm_log(payload->handle, PM_LOG_ERROR, _("url '%s' is invalid\n"), payload->fileurl); + RET_ERR(payload->handle, PM_ERR_SERVER_BAD_URL, -1); }

if(strlen(dlfile.filename) > 0 && strcmp(dlfile.filename, ".sig") != 0) { @@ -226,12 +225,12 @@ static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *p if((fd = mkstemp(randpath)) == -1 || !(localf = fdopen(fd, open_mode))) { unlink(randpath); close(fd); - _alpm_log(handle, PM_LOG_ERROR, + _alpm_log(payload->handle, PM_LOG_ERROR, _("failed to create temporary file for download\n")); goto cleanup; } /* localf now points to our alpmtmp.XXXXXX */ - STRDUP(tempfile, randpath, RET_ERR(handle, PM_ERR_MEMORY, -1)); + STRDUP(tempfile, randpath, RET_ERR(payload->handle, PM_ERR_MEMORY, -1)); dlfile.filename = strrchr(randpath, '/') + 1; }

@@ -239,39 +238,39 @@ static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *p

/* the curl_easy handle is initialized with the alpm handle, so we only need * to reset the curl handle set parameters for each time it's used. */ - curl_easy_reset(handle->curl); - curl_easy_setopt(handle->curl, CURLOPT_URL, payload->fileurl); - curl_easy_setopt(handle->curl, CURLOPT_FAILONERROR, 1L); - curl_easy_setopt(handle->curl, CURLOPT_ERRORBUFFER, error_buffer); - curl_easy_setopt(handle->curl, CURLOPT_CONNECTTIMEOUT, 10L); - curl_easy_setopt(handle->curl, CURLOPT_FILETIME, 1L); - curl_easy_setopt(handle->curl, CURLOPT_NOPROGRESS, 0L); - curl_easy_setopt(handle->curl, CURLOPT_FOLLOWLOCATION, 1L); - curl_easy_setopt(handle->curl, CURLOPT_PROGRESSFUNCTION, curl_progress); - curl_easy_setopt(handle->curl, CURLOPT_PROGRESSDATA, (void *)&dlfile); - curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_LIMIT, 1024L); - curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_TIME, 10L); - curl_easy_setopt(handle->curl, CURLOPT_HEADERFUNCTION, parse_headers); - curl_easy_setopt(handle->curl, CURLOPT_WRITEHEADER, &dlfile); + curl_easy_reset(payload->handle->curl); + curl_easy_setopt(payload->handle->curl, CURLOPT_URL, payload->fileurl); + curl_easy_setopt(payload->handle->curl, CURLOPT_FAILONERROR, 1L); + curl_easy_setopt(payload->handle->curl, CURLOPT_ERRORBUFFER, error_buffer); + curl_easy_setopt(payload->handle->curl, CURLOPT_CONNECTTIMEOUT, 10L); + curl_easy_setopt(payload->handle->curl, CURLOPT_FILETIME, 1L); + curl_easy_setopt(payload->handle->curl, CURLOPT_NOPROGRESS, 0L); + curl_easy_setopt(payload->handle->curl, CURLOPT_FOLLOWLOCATION, 1L); + curl_easy_setopt(payload->handle->curl, CURLOPT_PROGRESSFUNCTION, curl_progress); + curl_easy_setopt(payload->handle->curl, CURLOPT_PROGRESSDATA, (void *)&dlfile); + curl_easy_setopt(payload->handle->curl, CURLOPT_LOW_SPEED_LIMIT, 1024L); + curl_easy_setopt(payload->handle->curl, CURLOPT_LOW_SPEED_TIME, 10L); + curl_easy_setopt(payload->handle->curl, CURLOPT_HEADERFUNCTION, parse_headers); + curl_easy_setopt(payload->handle->curl, CURLOPT_WRITEHEADER, &dlfile);

if(payload->max_size) { - curl_easy_setopt(handle->curl, CURLOPT_MAXFILESIZE, payload->max_size); + curl_easy_setopt(payload->handle->curl, CURLOPT_MAXFILESIZE, payload->max_size); }

useragent = getenv("HTTP_USER_AGENT"); if(useragent != NULL) { - curl_easy_setopt(handle->curl, CURLOPT_USERAGENT, useragent); + curl_easy_setopt(payload->handle->curl, CURLOPT_USERAGENT, useragent); }

- if(!allow_resume && !force && stat(destfile, &st) == 0) { + if(!payload->allow_resume && !payload->force && stat(destfile, &st) == 0) { /* start from scratch, but only download if our local is out of date. */ - curl_easy_setopt(handle->curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_IFMODSINCE); - curl_easy_setopt(handle->curl, CURLOPT_TIMEVALUE, (long)st.st_mtime); - } else if(stat(tempfile, &st) == 0 && allow_resume) { + curl_easy_setopt(payload->handle->curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_IFMODSINCE); + curl_easy_setopt(payload->handle->curl, CURLOPT_TIMEVALUE, (long)st.st_mtime); + } else if(stat(tempfile, &st) == 0 && payload->allow_resume) { /* a previous partial download exists, resume from end of file. */ open_mode = "ab"; - curl_easy_setopt(handle->curl, CURLOPT_RESUME_FROM, (long)st.st_size); - _alpm_log(handle, PM_LOG_DEBUG, "tempfile found, attempting continuation"); + curl_easy_setopt(payload->handle->curl, CURLOPT_RESUME_FROM, (long)st.st_size); + _alpm_log(payload->handle, PM_LOG_DEBUG, "tempfile found, attempting continuation"); dlfile.initial_size = (double)st.st_size; }

@@ -282,7 +281,7 @@ static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *p } }

- curl_easy_setopt(handle->curl, CURLOPT_WRITEDATA, localf); + curl_easy_setopt(payload->handle->curl, CURLOPT_WRITEDATA, localf);

/* ignore any SIGPIPE signals- these may occur if our FTP socket dies or * something along those lines. Store the old signal handler first. */ @@ -303,18 +302,18 @@ static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *p prevprogress = 0;

/* perform transfer */ - handle->curlerr = curl_easy_perform(handle->curl); + payload->handle->curlerr = curl_easy_perform(payload->handle->curl);

/* was it a success? */ - if(handle->curlerr == CURLE_ABORTED_BY_CALLBACK) { + if(payload->handle->curlerr == CURLE_ABORTED_BY_CALLBACK) { goto cleanup; - } else if(handle->curlerr != CURLE_OK) { - if(!errors_ok) { - handle->pm_errno = PM_ERR_LIBCURL; - _alpm_log(handle, PM_LOG_ERROR, _("failed retrieving file '%s' from %s : %s\n"), + } else if(payload->handle->curlerr != CURLE_OK) { + if(!payload->errors_ok) { + payload->handle->pm_errno = PM_ERR_LIBCURL; + _alpm_log(payload->handle, PM_LOG_ERROR, _("failed retrieving file '%s' from %s : %s\n"), dlfile.filename, hostname, error_buffer); } else { - _alpm_log(handle, PM_LOG_DEBUG, "failed retrieving file '%s' from %s : %s\n", + _alpm_log(payload->handle, PM_LOG_DEBUG, "failed retrieving file '%s' from %s : %s\n", dlfile.filename, hostname, error_buffer); } unlink(tempfile); @@ -322,11 +321,11 @@ static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *p }

/* retrieve info about the state of the transfer */ - curl_easy_getinfo(handle->curl, CURLINFO_FILETIME, &remote_time); - curl_easy_getinfo(handle->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &remote_size); - curl_easy_getinfo(handle->curl, CURLINFO_SIZE_DOWNLOAD, &bytes_dl); - curl_easy_getinfo(handle->curl, CURLINFO_CONDITION_UNMET, &timecond); - curl_easy_getinfo(handle->curl, CURLINFO_EFFECTIVE_URL, &effective_url); + curl_easy_getinfo(payload->handle->curl, CURLINFO_FILETIME, &remote_time); + curl_easy_getinfo(payload->handle->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &remote_size); + curl_easy_getinfo(payload->handle->curl, CURLINFO_SIZE_DOWNLOAD, &bytes_dl); + curl_easy_getinfo(payload->handle->curl, CURLINFO_CONDITION_UNMET, &timecond); + curl_easy_getinfo(payload->handle->curl, CURLINFO_EFFECTIVE_URL, &effective_url);

/* time condition was met and we didn't download anything. we need to * clean up the 0 byte .part file that's left behind. */ @@ -341,8 +340,8 @@ static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *p * as actually being transferred during curl_easy_perform() */ if(!DOUBLE_EQ(remote_size, -1) && !DOUBLE_EQ(bytes_dl, -1) && !DOUBLE_EQ(bytes_dl, remote_size)) { - handle->pm_errno = PM_ERR_RETRIEVE; - _alpm_log(handle, PM_LOG_ERROR, _("%s appears to be truncated: %jd/%jd bytes\n"), + payload->handle->pm_errno = PM_ERR_RETRIEVE; + _alpm_log(payload->handle, PM_LOG_ERROR, _("%s appears to be truncated: %jd/%jd bytes\n"), dlfile.filename, (intmax_t)bytes_dl, (intmax_t)remote_size); goto cleanup; } @@ -413,21 +412,19 @@ cleanup: * @param errors_ok do not log errors (but still return them) * @return 0 on success, -1 on error (pm_errno is set accordingly if errors_ok == 0) */ -int _alpm_download(alpm_handle_t *handle, struct dload_payload *payload, - const char *localpath, char **final_file, int force, int allow_resume, - int errors_ok) +int _alpm_download(struct dload_payload *payload, const char *localpath, + char **final_file) { - if(handle->fetchcb == NULL) { + if(payload->handle->fetchcb == NULL) { #ifdef HAVE_LIBCURL - return curl_download_internal(handle, payload, localpath, final_file, force, - allow_resume, errors_ok); + return curl_download_internal(payload, localpath, final_file); #else - RET_ERR(handle, PM_ERR_EXTERNAL_DOWNLOAD, -1); + RET_ERR(payload->handle, PM_ERR_EXTERNAL_DOWNLOAD, -1); #endif } else { - int ret = handle->fetchcb(payload->fileurl, localpath, force); - if(ret == -1 && !errors_ok) { - RET_ERR(handle, PM_ERR_EXTERNAL_DOWNLOAD, -1); + int ret = payload->handle->fetchcb(payload->fileurl, localpath, payload->force); + if(ret == -1 && !payload->errors_ok) { + RET_ERR(payload->handle, PM_ERR_EXTERNAL_DOWNLOAD, -1); } return ret; } @@ -449,9 +446,10 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url)

CALLOC(payload, 1, sizeof(*payload), RET_ERR(handle, PM_ERR_MEMORY, NULL)); payload->filename = strdup(url);

Dan style typo here -- should be fileurl, not filename. I also never assign the handle, leading to a lovely null deref. Fixed in git.

+ payload->allow_resume = 1;

/* download the file */ - ret = _alpm_download(handle, payload, cachedir, &final_file, 0, 1, 0); + ret = _alpm_download(payload, cachedir, &final_file); if(ret == -1) { _alpm_log(handle, PM_LOG_WARNING, _("failed to download %s\n"), url); return NULL; @@ -462,16 +460,18 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url) if(ret == 0 && (handle->sigverify == PM_PGP_VERIFY_ALWAYS || handle->sigverify == PM_PGP_VERIFY_OPTIONAL)) { size_t len; - int errors_ok = (handle->sigverify == PM_PGP_VERIFY_OPTIONAL); struct dload_payload *spayload;

CALLOC(spayload, 1, sizeof(*spayload), RET_ERR(handle, PM_ERR_MEMORY, NULL)); len = strlen(url) + 5; CALLOC(spayload->fileurl, len, sizeof(char), RET_ERR(handle, PM_ERR_MEMORY, NULL)); snprintf(spayload->fileurl, len, "%s.sig", url); + spayload->handle = handle; + spayload->force = 1; + spayload->errors_ok = (handle->sigverify == PM_PGP_VERIFY_OPTIONAL);

- ret = _alpm_download(handle, spayload, cachedir, &final_file, 1, 0, errors_ok); - if(ret == -1 && !errors_ok) { + ret = _alpm_download(spayload, cachedir, &final_file); + if(ret == -1 && !spayload->errors_ok) { _alpm_log(handle, PM_LOG_WARNING, _("failed to download %s\n"), spayload->fileurl); /* Warn now, but don't return NULL. We will fail later during package * load time. */ diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h index 8aaed0c..19bd499 100644 --- a/lib/libalpm/dload.h +++ b/lib/libalpm/dload.h @@ -34,16 +34,19 @@ struct fileinfo { };

struct dload_payload { + alpm_handle_t *handle; char *filename; char *fileurl; long max_size; + int force; + int allow_resume; + int errors_ok; };

void _alpm_dload_payload_free(void *payload);

-int _alpm_download(alpm_handle_t *handle, struct dload_payload *payload, - const char *localpath, char **final_file, int force, int allow_resume, - int errors_ok); +int _alpm_download(struct dload_payload *payload, const char *localpath, + char **final_file);

#endif /* _ALPM_DLOAD_H */

diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 4277283..f63e171 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -804,8 +804,10 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas) len = strlen(server_url) + strlen(payload->filename) + 2; CALLOC(payload->fileurl, len, sizeof(char), RET_ERR(handle, PM_ERR_MEMORY, -1)); snprintf(payload->fileurl, len, "%s/%s", server_url, payload->filename); + payload->handle = handle; + payload->allow_resume = 1;

- ret = _alpm_download(handle, payload, cachedir, NULL, 0, 1, 0); + ret = _alpm_download(payload, cachedir, NULL); FREE(fileurl); if(ret != -1) { break; -- 1.7.6

On Sun, Jul 03, 2011 at 02:20:13PM -0500, Dan McGee wrote:

On Fri, Jul 1, 2011 at 7:59 AM, Dave Reisner <d@falconindy.com> wrote:

...

Restore some sanity to the number of arguments passed to _alpm_download and curl_download_internal.

Signed-off-by: Dave Reisner <dreisner@archlinux.org> ---  lib/libalpm/be_sync.c |   12 +++--  lib/libalpm/dload.c   |  118 ++++++++++++++++++++++++------------------------  lib/libalpm/dload.h   |    9 +++-  lib/libalpm/sync.c    |    4 +-  4 files changed, 76 insertions(+), 67 deletions(-)

diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 65420cf..9a3046a 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -188,8 +188,10 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)                len = strlen(server) + strlen(db->treename) + 9;                CALLOC(payload->fileurl, len, sizeof(char), RET_ERR(handle, PM_ERR_MEMORY, -1));                snprintf(payload->fileurl, len, "%s/%s.db", server, db->treename); +               payload->handle = handle; +               payload->force = force;

-               ret = _alpm_download(handle, payload, syncpath, NULL, force, 0, 0); +               ret = _alpm_download(payload, syncpath, NULL); If we're going to go so far as putting the handle in the payload, why not these last tow things as well?

I considered doing that, but thought it to be a bit neurotic. I guess it makes sense, though.

...

               if(ret == 0 && (check_sig == PM_PGP_VERIFY_ALWAYS ||                                        check_sig == PM_PGP_VERIFY_OPTIONAL)) { @@ -202,16 +204,18 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)                        unlink(sigpath);                        free(sigpath);

-                       int errors_ok = (check_sig == PM_PGP_VERIFY_OPTIONAL);                        /* if we downloaded a DB, we want the .sig from the same server */                        snprintf(payload->fileurl, len, "%s/%s.db.sig", server, db->treename); +                       payload->handle = handle; +                       payload->force = 1; +                       payload->errors_ok = (check_sig == PM_PGP_VERIFY_OPTIONAL);

                       /* set hard upper limited of 16KiB */                        payload->max_size = 16 * 1024;

-                       sig_ret = _alpm_download(handle, payload, syncpath, NULL, 1, 0, errors_ok); +                       sig_ret = _alpm_download(payload, syncpath, NULL);                        /* errors_ok suppresses error messages, but not the return code */ -                       sig_ret = errors_ok ? 0 : sig_ret; +                       sig_ret = payload->errors_ok ? 0 : sig_ret;                }

               _alpm_dload_payload_free(payload); diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 1758744..bac9356 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -179,9 +179,8 @@ static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user)        return realsize;  }

-static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *payload, -               const char *localpath, char **final_file, int force, int allow_resume, -               int errors_ok) +static int curl_download_internal(struct dload_payload *payload, +               const char *localpath, char **final_file)  {        int ret = -1, should_unlink = 0;        FILE *localf = NULL; @@ -197,13 +196,13 @@ static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *p        struct sigaction sig_pipe[2], sig_int[2];        struct fileinfo dlfile;

-       dlfile.handle = handle; +       dlfile.handle = payload->handle;        dlfile.initial_size = 0.0;        dlfile.filename = get_filename(payload->fileurl);        dlfile.cd_filename = NULL;

What about absorbing dlfile into payload? Any reason they can't be the same struct?

Next patch.

...

       if(!dlfile.filename || curl_gethost(payload->fileurl, hostname) != 0) { -               _alpm_log(handle, PM_LOG_ERROR, _("url '%s' is invalid\n"), payload->fileurl); -               RET_ERR(handle, PM_ERR_SERVER_BAD_URL, -1); +               _alpm_log(payload->handle, PM_LOG_ERROR, _("url '%s' is invalid\n"), payload->fileurl); +               RET_ERR(payload->handle, PM_ERR_SERVER_BAD_URL, -1);        }

       if(strlen(dlfile.filename) > 0 && strcmp(dlfile.filename, ".sig") != 0) { @@ -226,12 +225,12 @@ static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *p                if((fd = mkstemp(randpath)) == -1 || !(localf = fdopen(fd, open_mode))) {                        unlink(randpath);                        close(fd); -                       _alpm_log(handle, PM_LOG_ERROR, +                       _alpm_log(payload->handle, PM_LOG_ERROR,                                        _("failed to create temporary file for download\n"));                        goto cleanup;                }                /* localf now points to our alpmtmp.XXXXXX */ -               STRDUP(tempfile, randpath, RET_ERR(handle, PM_ERR_MEMORY, -1)); +               STRDUP(tempfile, randpath, RET_ERR(payload->handle, PM_ERR_MEMORY, -1));                dlfile.filename = strrchr(randpath, '/') + 1;        }

@@ -239,39 +238,39 @@ static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *p

       /* the curl_easy handle is initialized with the alpm handle, so we only need         * to reset the curl handle set parameters for each time it's used. */ I'd keep a pointer around to handle to skip having to change all this garbage and introduce noise- the compiler won't produce any different code and it makes it easier for us to read. I did this in just about any function that needed reference to the handle more than 3 times.

I'll wait to review this patch until that change; otherwise the signal/noise ratio is really high.

Good call, this definitely cleans up the patch a lot.

...

-       curl_easy_reset(handle->curl); -       curl_easy_setopt(handle->curl, CURLOPT_URL, payload->fileurl); -       curl_easy_setopt(handle->curl, CURLOPT_FAILONERROR, 1L); -       curl_easy_setopt(handle->curl, CURLOPT_ERRORBUFFER, error_buffer); -       curl_easy_setopt(handle->curl, CURLOPT_CONNECTTIMEOUT, 10L); -       curl_easy_setopt(handle->curl, CURLOPT_FILETIME, 1L); -       curl_easy_setopt(handle->curl, CURLOPT_NOPROGRESS, 0L); -       curl_easy_setopt(handle->curl, CURLOPT_FOLLOWLOCATION, 1L); -       curl_easy_setopt(handle->curl, CURLOPT_PROGRESSFUNCTION, curl_progress); -       curl_easy_setopt(handle->curl, CURLOPT_PROGRESSDATA, (void *)&dlfile); -       curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_LIMIT, 1024L); -       curl_easy_setopt(handle->curl, CURLOPT_LOW_SPEED_TIME, 10L); -       curl_easy_setopt(handle->curl, CURLOPT_HEADERFUNCTION, parse_headers); -       curl_easy_setopt(handle->curl, CURLOPT_WRITEHEADER, &dlfile); +       curl_easy_reset(payload->handle->curl); +       curl_easy_setopt(payload->handle->curl, CURLOPT_URL, payload->fileurl); +       curl_easy_setopt(payload->handle->curl, CURLOPT_FAILONERROR, 1L); +       curl_easy_setopt(payload->handle->curl, CURLOPT_ERRORBUFFER, error_buffer); +       curl_easy_setopt(payload->handle->curl, CURLOPT_CONNECTTIMEOUT, 10L); +       curl_easy_setopt(payload->handle->curl, CURLOPT_FILETIME, 1L); +       curl_easy_setopt(payload->handle->curl, CURLOPT_NOPROGRESS, 0L); +       curl_easy_setopt(payload->handle->curl, CURLOPT_FOLLOWLOCATION, 1L); +       curl_easy_setopt(payload->handle->curl, CURLOPT_PROGRESSFUNCTION, curl_progress); +       curl_easy_setopt(payload->handle->curl, CURLOPT_PROGRESSDATA, (void *)&dlfile); +       curl_easy_setopt(payload->handle->curl, CURLOPT_LOW_SPEED_LIMIT, 1024L); +       curl_easy_setopt(payload->handle->curl, CURLOPT_LOW_SPEED_TIME, 10L); +       curl_easy_setopt(payload->handle->curl, CURLOPT_HEADERFUNCTION, parse_headers); +       curl_easy_setopt(payload->handle->curl, CURLOPT_WRITEHEADER, &dlfile);

       if(payload->max_size) { -               curl_easy_setopt(handle->curl, CURLOPT_MAXFILESIZE, payload->max_size); +               curl_easy_setopt(payload->handle->curl, CURLOPT_MAXFILESIZE, payload->max_size);        }

       useragent = getenv("HTTP_USER_AGENT");        if(useragent != NULL) { -               curl_easy_setopt(handle->curl, CURLOPT_USERAGENT, useragent); +               curl_easy_setopt(payload->handle->curl, CURLOPT_USERAGENT, useragent);        }

-       if(!allow_resume && !force && stat(destfile, &st) == 0) { +       if(!payload->allow_resume && !payload->force && stat(destfile, &st) == 0) {                /* start from scratch, but only download if our local is out of date. */ -               curl_easy_setopt(handle->curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_IFMODSINCE); -               curl_easy_setopt(handle->curl, CURLOPT_TIMEVALUE, (long)st.st_mtime); -       } else if(stat(tempfile, &st) == 0 && allow_resume) { +               curl_easy_setopt(payload->handle->curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_IFMODSINCE); +               curl_easy_setopt(payload->handle->curl, CURLOPT_TIMEVALUE, (long)st.st_mtime); +       } else if(stat(tempfile, &st) == 0 && payload->allow_resume) {                /* a previous partial download exists, resume from end of file. */                open_mode = "ab"; -               curl_easy_setopt(handle->curl, CURLOPT_RESUME_FROM, (long)st.st_size); -               _alpm_log(handle, PM_LOG_DEBUG, "tempfile found, attempting continuation"); +               curl_easy_setopt(payload->handle->curl, CURLOPT_RESUME_FROM, (long)st.st_size); +               _alpm_log(payload->handle, PM_LOG_DEBUG, "tempfile found, attempting continuation");                dlfile.initial_size = (double)st.st_size;        }

@@ -282,7 +281,7 @@ static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *p                }        }

-       curl_easy_setopt(handle->curl, CURLOPT_WRITEDATA, localf); +       curl_easy_setopt(payload->handle->curl, CURLOPT_WRITEDATA, localf);

       /* ignore any SIGPIPE signals- these may occur if our FTP socket dies or         * something along those lines. Store the old signal handler first. */ @@ -303,18 +302,18 @@ static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *p        prevprogress = 0;

       /* perform transfer */ -       handle->curlerr = curl_easy_perform(handle->curl); +       payload->handle->curlerr = curl_easy_perform(payload->handle->curl);

       /* was it a success? */ -       if(handle->curlerr == CURLE_ABORTED_BY_CALLBACK) { +       if(payload->handle->curlerr == CURLE_ABORTED_BY_CALLBACK) {                goto cleanup; -       } else if(handle->curlerr != CURLE_OK) { -               if(!errors_ok) { -                       handle->pm_errno = PM_ERR_LIBCURL; -                       _alpm_log(handle, PM_LOG_ERROR, _("failed retrieving file '%s' from %s : %s\n"), +       } else if(payload->handle->curlerr != CURLE_OK) { +               if(!payload->errors_ok) { +                       payload->handle->pm_errno = PM_ERR_LIBCURL; +                       _alpm_log(payload->handle, PM_LOG_ERROR, _("failed retrieving file '%s' from %s : %s\n"),                                        dlfile.filename, hostname, error_buffer);                } else { -                       _alpm_log(handle, PM_LOG_DEBUG, "failed retrieving file '%s' from %s : %s\n", +                       _alpm_log(payload->handle, PM_LOG_DEBUG, "failed retrieving file '%s' from %s : %s\n",                                        dlfile.filename, hostname, error_buffer);                }                unlink(tempfile); @@ -322,11 +321,11 @@ static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *p        }

       /* retrieve info about the state of the transfer */ -       curl_easy_getinfo(handle->curl, CURLINFO_FILETIME, &remote_time); -       curl_easy_getinfo(handle->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &remote_size); -       curl_easy_getinfo(handle->curl, CURLINFO_SIZE_DOWNLOAD, &bytes_dl); -       curl_easy_getinfo(handle->curl, CURLINFO_CONDITION_UNMET, &timecond); -       curl_easy_getinfo(handle->curl, CURLINFO_EFFECTIVE_URL, &effective_url); +       curl_easy_getinfo(payload->handle->curl, CURLINFO_FILETIME, &remote_time); +       curl_easy_getinfo(payload->handle->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &remote_size); +       curl_easy_getinfo(payload->handle->curl, CURLINFO_SIZE_DOWNLOAD, &bytes_dl); +       curl_easy_getinfo(payload->handle->curl, CURLINFO_CONDITION_UNMET, &timecond); +       curl_easy_getinfo(payload->handle->curl, CURLINFO_EFFECTIVE_URL, &effective_url);

       /* time condition was met and we didn't download anything. we need to         * clean up the 0 byte .part file that's left behind. */ @@ -341,8 +340,8 @@ static int curl_download_internal(alpm_handle_t *handle, struct dload_payload *p         * as actually being transferred during curl_easy_perform() */        if(!DOUBLE_EQ(remote_size, -1) && !DOUBLE_EQ(bytes_dl, -1) &&                        !DOUBLE_EQ(bytes_dl, remote_size)) { -               handle->pm_errno = PM_ERR_RETRIEVE; -               _alpm_log(handle, PM_LOG_ERROR, _("%s appears to be truncated: %jd/%jd bytes\n"), +               payload->handle->pm_errno = PM_ERR_RETRIEVE; +               _alpm_log(payload->handle, PM_LOG_ERROR, _("%s appears to be truncated: %jd/%jd bytes\n"),                                dlfile.filename, (intmax_t)bytes_dl, (intmax_t)remote_size);                goto cleanup;        } @@ -413,21 +412,19 @@ cleanup:  * @param errors_ok do not log errors (but still return them)  * @return 0 on success, -1 on error (pm_errno is set accordingly if errors_ok == 0)  */ -int _alpm_download(alpm_handle_t *handle, struct dload_payload *payload, -               const char *localpath, char **final_file, int force, int allow_resume, -               int errors_ok) +int _alpm_download(struct dload_payload *payload, const char *localpath, +               char **final_file)  { -       if(handle->fetchcb == NULL) { +       if(payload->handle->fetchcb == NULL) {  #ifdef HAVE_LIBCURL -               return curl_download_internal(handle, payload, localpath, final_file, force, -                               allow_resume, errors_ok); +               return curl_download_internal(payload, localpath, final_file);  #else -               RET_ERR(handle, PM_ERR_EXTERNAL_DOWNLOAD, -1); +               RET_ERR(payload->handle, PM_ERR_EXTERNAL_DOWNLOAD, -1);  #endif        } else { -               int ret = handle->fetchcb(payload->fileurl, localpath, force); -               if(ret == -1 && !errors_ok) { -                       RET_ERR(handle, PM_ERR_EXTERNAL_DOWNLOAD, -1); +               int ret = payload->handle->fetchcb(payload->fileurl, localpath, payload->force); +               if(ret == -1 && !payload->errors_ok) { +                       RET_ERR(payload->handle, PM_ERR_EXTERNAL_DOWNLOAD, -1);                }                return ret;        } @@ -449,9 +446,10 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url)

       CALLOC(payload, 1, sizeof(*payload), RET_ERR(handle, PM_ERR_MEMORY, NULL));        payload->filename = strdup(url); +       payload->allow_resume = 1;

       /* download the file */ -       ret = _alpm_download(handle, payload, cachedir, &final_file, 0, 1, 0); +       ret = _alpm_download(payload, cachedir, &final_file);        if(ret == -1) {                _alpm_log(handle, PM_LOG_WARNING, _("failed to download %s\n"), url);                return NULL; @@ -462,16 +460,18 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url)        if(ret == 0 && (handle->sigverify == PM_PGP_VERIFY_ALWAYS ||                                handle->sigverify == PM_PGP_VERIFY_OPTIONAL)) {                size_t len; -               int errors_ok = (handle->sigverify == PM_PGP_VERIFY_OPTIONAL);                struct dload_payload *spayload;

               CALLOC(spayload, 1, sizeof(*spayload), RET_ERR(handle, PM_ERR_MEMORY, NULL));                len = strlen(url) + 5;                CALLOC(spayload->fileurl, len, sizeof(char), RET_ERR(handle, PM_ERR_MEMORY, NULL));                snprintf(spayload->fileurl, len, "%s.sig", url); +               spayload->handle = handle; +               spayload->force = 1; +               spayload->errors_ok = (handle->sigverify == PM_PGP_VERIFY_OPTIONAL);

-               ret = _alpm_download(handle, spayload, cachedir, &final_file, 1, 0, errors_ok); -               if(ret == -1 && !errors_ok) { +               ret = _alpm_download(spayload, cachedir, &final_file); +               if(ret == -1 && !spayload->errors_ok) {                        _alpm_log(handle, PM_LOG_WARNING, _("failed to download %s\n"), spayload->fileurl);                        /* Warn now, but don't return NULL. We will fail later during package                         * load time. */ diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h index 8aaed0c..19bd499 100644 --- a/lib/libalpm/dload.h +++ b/lib/libalpm/dload.h @@ -34,16 +34,19 @@ struct fileinfo {  };

 struct dload_payload { +       alpm_handle_t *handle;        char *filename;        char *fileurl;        long max_size; +       int force; +       int allow_resume; +       int errors_ok;  };

 void _alpm_dload_payload_free(void *payload);

-int _alpm_download(alpm_handle_t *handle, struct dload_payload *payload, -               const char *localpath, char **final_file, int force, int allow_resume, -               int errors_ok); +int _alpm_download(struct dload_payload *payload, const char *localpath, +               char **final_file);

 #endif /* _ALPM_DLOAD_H */

diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 4277283..f63e171 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -804,8 +804,10 @@ static int download_files(alpm_handle_t *handle, alpm_list_t **deltas)                                        len = strlen(server_url) + strlen(payload->filename) + 2;                                        CALLOC(payload->fileurl, len, sizeof(char), RET_ERR(handle, PM_ERR_MEMORY, -1));                                        snprintf(payload->fileurl, len, "%s/%s", server_url, payload->filename); +                                       payload->handle = handle; +                                       payload->allow_resume = 1;

-                                       ret = _alpm_download(handle, payload, cachedir, NULL, 0, 1, 0); +                                       ret = _alpm_download(payload, cachedir, NULL);                                        FREE(fileurl);                                        if(ret != -1) {                                                break; -- 1.7.6

On Fri, Jul 1, 2011 at 7:59 AM, Dave Reisner <d@falconindy.com> wrote:

This transitional struct becomes delicious noms for dload_payload. Ignore me on the previous patch; merging the two is better done in a separate patch, so keep it like this.

Signed-off-by: Dave Reisner <dreisner@archlinux.org> ---  lib/libalpm/dload.c |   57 ++++++++++++++++++++++++--------------------------  lib/libalpm/dload.h |   10 +-------  2 files changed, 29 insertions(+), 38 deletions(-)

diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index bac9356..f41e395 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -46,7 +46,7 @@  static double prevprogress; /* last download amount */  #endif

-static const char *get_filename(const char *url) +static char *get_filename(const char *url) explanation?

 {        char *filename = strrchr(url, '/');        if(filename != NULL) { @@ -80,7 +80,7 @@ static void inthandler(int UNUSED signum)  static int curl_progress(void *file, double dltotal, double dlnow,                double UNUSED ultotal, double UNUSED ulnow)  { -       struct fileinfo *dlfile = (struct fileinfo *)file; +       struct dload_payload *payload = (struct dload_payload *)file;        double current_size, total_size;

       /* SIGINT sent, abort by alerting curl */ @@ -89,12 +89,12 @@ static int curl_progress(void *file, double dltotal, double dlnow,        }

       /* none of what follows matters if the front end has no callback */ -       if(dlfile->handle->dlcb == NULL) { +       if(payload->handle->dlcb == NULL) {                return 0;        }

-       current_size = dlfile->initial_size + dlnow; -       total_size = dlfile->initial_size + dltotal; +       current_size = payload->initial_size + dlnow; +       total_size = payload->initial_size + dltotal;

       if(DOUBLE_EQ(dltotal, 0) || DOUBLE_EQ(prevprogress, total_size)) {                return 0; @@ -103,10 +103,10 @@ static int curl_progress(void *file, double dltotal, double dlnow,        /* initialize the progress bar here to avoid displaying it when         * a repo is up to date and nothing gets downloaded */        if(DOUBLE_EQ(prevprogress, 0)) { -               dlfile->handle->dlcb(dlfile->filename, 0, (long)dltotal); +               payload->handle->dlcb(payload->filename, 0, (long)dltotal);        }

-       dlfile->handle->dlcb(dlfile->filename, (long)current_size, (long)total_size); +       payload->handle->dlcb(payload->filename, (long)current_size, (long)total_size);

       prevprogress = current_size;

@@ -154,7 +154,7 @@ static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user)        const char *fptr, *endptr = NULL;        const char * const cd_header = "Content-Disposition:";        const char * const fn_key = "filename="; -       struct fileinfo **dlfile = (struct fileinfo**)user; +       struct dload_payload *payload = (struct dload_payload *)user;

       if(strncasecmp(cd_header, ptr, strlen(cd_header)) == 0) {                if((fptr = strstr(ptr, fn_key))) { @@ -171,8 +171,8 @@ static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user)                                endptr--;                        }

-                       STRNDUP((*dlfile)->cd_filename, fptr, endptr - fptr + 1, -                                       RET_ERR((*dlfile)->handle, PM_ERR_MEMORY, realsize)); +                       STRNDUP(payload->cd_filename, fptr, endptr - fptr + 1, +                                       RET_ERR(payload->handle, PM_ERR_MEMORY, realsize));                }        }

@@ -194,20 +194,18 @@ static int curl_download_internal(struct dload_payload *payload,        long timecond, remote_time = -1;        double remote_size, bytes_dl;        struct sigaction sig_pipe[2], sig_int[2]; -       struct fileinfo dlfile;

-       dlfile.handle = payload->handle; -       dlfile.initial_size = 0.0; -       dlfile.filename = get_filename(payload->fileurl); -       dlfile.cd_filename = NULL; -       if(!dlfile.filename || curl_gethost(payload->fileurl, hostname) != 0) { +       if(!payload->filename) { +               payload->filename = get_filename(payload->fileurl); +       } +       if(!payload->filename || curl_gethost(payload->fileurl, hostname) != 0) {                _alpm_log(payload->handle, PM_LOG_ERROR, _("url '%s' is invalid\n"), payload->fileurl);                RET_ERR(payload->handle, PM_ERR_SERVER_BAD_URL, -1);        }

-       if(strlen(dlfile.filename) > 0 && strcmp(dlfile.filename, ".sig") != 0) { -               destfile = get_fullpath(localpath, dlfile.filename, ""); -               tempfile = get_fullpath(localpath, dlfile.filename, ".part"); +       if(strlen(payload->filename) > 0 && strcmp(payload->filename, ".sig") != 0) { +               destfile = get_fullpath(localpath, payload->filename, ""); +               tempfile = get_fullpath(localpath, payload->filename, ".part");                if(!destfile || !tempfile) {                        goto cleanup;                } @@ -231,7 +229,7 @@ static int curl_download_internal(struct dload_payload *payload,                }                /* localf now points to our alpmtmp.XXXXXX */                STRDUP(tempfile, randpath, RET_ERR(payload->handle, PM_ERR_MEMORY, -1)); -               dlfile.filename = strrchr(randpath, '/') + 1; +               payload->filename = strrchr(randpath, '/') + 1;        }

       error_buffer[0] = '\0'; @@ -247,11 +245,11 @@ static int curl_download_internal(struct dload_payload *payload,        curl_easy_setopt(payload->handle->curl, CURLOPT_NOPROGRESS, 0L);        curl_easy_setopt(payload->handle->curl, CURLOPT_FOLLOWLOCATION, 1L);        curl_easy_setopt(payload->handle->curl, CURLOPT_PROGRESSFUNCTION, curl_progress); -       curl_easy_setopt(payload->handle->curl, CURLOPT_PROGRESSDATA, (void *)&dlfile); +       curl_easy_setopt(payload->handle->curl, CURLOPT_PROGRESSDATA, (void *)payload);        curl_easy_setopt(payload->handle->curl, CURLOPT_LOW_SPEED_LIMIT, 1024L);        curl_easy_setopt(payload->handle->curl, CURLOPT_LOW_SPEED_TIME, 10L);        curl_easy_setopt(payload->handle->curl, CURLOPT_HEADERFUNCTION, parse_headers); -       curl_easy_setopt(payload->handle->curl, CURLOPT_WRITEHEADER, &dlfile); +       curl_easy_setopt(payload->handle->curl, CURLOPT_WRITEHEADER, (void *)payload);

       if(payload->max_size) {                curl_easy_setopt(payload->handle->curl, CURLOPT_MAXFILESIZE, payload->max_size); @@ -271,7 +269,7 @@ static int curl_download_internal(struct dload_payload *payload,                open_mode = "ab";                curl_easy_setopt(payload->handle->curl, CURLOPT_RESUME_FROM, (long)st.st_size);                _alpm_log(payload->handle, PM_LOG_DEBUG, "tempfile found, attempting continuation"); -               dlfile.initial_size = (double)st.st_size; +               payload->initial_size = (double)st.st_size;        }

       if(localf == NULL) { @@ -311,10 +309,10 @@ static int curl_download_internal(struct dload_payload *payload,                if(!payload->errors_ok) {                        payload->handle->pm_errno = PM_ERR_LIBCURL;                        _alpm_log(payload->handle, PM_LOG_ERROR, _("failed retrieving file '%s' from %s : %s\n"), -                                       dlfile.filename, hostname, error_buffer); +                                       payload->filename, hostname, error_buffer);                } else {                        _alpm_log(payload->handle, PM_LOG_DEBUG, "failed retrieving file '%s' from %s : %s\n", -                                       dlfile.filename, hostname, error_buffer); +                                       payload->filename, hostname, error_buffer);                }                unlink(tempfile);                goto cleanup; @@ -342,14 +340,14 @@ static int curl_download_internal(struct dload_payload *payload,                        !DOUBLE_EQ(bytes_dl, remote_size)) {                payload->handle->pm_errno = PM_ERR_RETRIEVE;                _alpm_log(payload->handle, PM_LOG_ERROR, _("%s appears to be truncated: %jd/%jd bytes\n"), -                               dlfile.filename, (intmax_t)bytes_dl, (intmax_t)remote_size); +                               payload->filename, (intmax_t)bytes_dl, (intmax_t)remote_size);                goto cleanup;        }

-       if(dlfile.cd_filename) { +       if(payload->cd_filename) {                /* content-disposition header has a better name for our file */                free(destfile); -               destfile = get_fullpath(localpath, dlfile.cd_filename, ""); +               destfile = get_fullpath(localpath, payload->cd_filename, "");        } else {                const char *effective_filename = strrchr(effective_url, '/');                if(effective_filename) { @@ -387,7 +385,6 @@ cleanup:

       FREE(tempfile);        FREE(destfile); -       FREE(dlfile.cd_filename);

       /* restore the old signal handlers */        sigaction(SIGINT, &sig_int[OLD], NULL); @@ -494,8 +491,8 @@ void _alpm_dload_payload_free(void *payload) { I missed this in an earlier patch, but I'd rather this match the sig of the rest of our free functions and take a typed pointer- that way, when used in a non list_free context, errors get caught. Instead, cast the function in list_free.

       ASSERT(load, return);

-       FREE(load->filename);        FREE(load->fileurl); +       FREE(load->cd_filename);        FREE(load);  }

diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h index 19bd499..db558be 100644 --- a/lib/libalpm/dload.h +++ b/lib/libalpm/dload.h @@ -25,18 +25,12 @@

 #include <time.h>

-/* internal structure for communicating with curl progress callback */ -struct fileinfo { -       alpm_handle_t *handle; -       const char *filename; -       char *cd_filename; -       double initial_size; -}; -  struct dload_payload {        alpm_handle_t *handle;        char *filename; +       char *cd_filename;        char *fileurl; +       double initial_size;        long max_size;        int force;        int allow_resume; -- 1.7.6