Re: [pacman-dev] [PATCH 3/4] absorb some _alpm_download params into payload struct

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

On Fri, Jul 1, 2011 at 7:59 AM, Dave Reisner wrote:

...

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

Signed-off-by: Dave Reisner ---  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