[pacman-dev] [PATCH] Add a fetch callback to allow front-end download support
Dan McGee
dpmcgee at gmail.com
Sat Mar 28 17:34:25 EDT 2009
On Fri, Feb 20, 2009 at 2:31 AM, Sebastian Nowicki <sebnow at gmail.com> wrote:
> This allows a frontend to define its own download algorithm so that the
> libfetch dependency can be omitted without using an external process. The
> callback will be used if it is defined, otherwise the internal method
> (libfetch) is used, if available.
>
> The external download method was moved to pacman and is set as the fetch
> callback, if the command is defined in the configuration file. As a result,
> alpm_option_get_xfercommand() and alpm_option_set_xfercommand() have been
> removed.
>
> Signed-off-by: Sebastian Nowicki <sebnow at gmail.com>
cc1: warnings being treated as errors
dload.c: In function ‘download’:
dload.c:274: error: passing argument 4 of ‘handle->fetchcb’ makes
pointer from integer without a cast
(you used mtimeold twice)
handle.c: In function ‘_alpm_handle_free’:
handle.c:81: error: ‘pmhandle_t’ has no member named ‘xfercommand’
handle.c:81: error: ‘pmhandle_t’ has no member named ‘xfercommand’
(just kill the line)
In file included from pacman.c:52:
conf.h:26:1: error: "PATH_MAX" redefined
In file included from /usr/include/bits/local_lim.h:39,
from /usr/include/bits/posix1_lim.h:153,
from /usr/include/limits.h:145,
from
/usr/lib/gcc/x86_64-unknown-linux-gnu/4.3.3/include-fixed/limits.h:122,
from
/usr/lib/gcc/x86_64-unknown-linux-gnu/4.3.3/include-fixed/syslimits.h:7,
from
/usr/lib/gcc/x86_64-unknown-linux-gnu/4.3.3/include-fixed/limits.h:11,
from pacman.c:30:
/usr/include/linux/limits.h:12:1: error: this is the location of the
previous definition
Don't mess with that path_max #ifdef junk- although it does look like
we interchange <sys/syslimits.h> and <limits.h> to get the definition
of PATH_MAX. Which one is correct?
Do you guys compile these patches first? :P
> ---
> lib/libalpm/alpm.h | 17 +++++++-
> lib/libalpm/dload.c | 101 +------------------------------------------------
> lib/libalpm/handle.c | 33 +++++++++-------
> lib/libalpm/handle.h | 2 +-
> src/pacman/conf.h | 6 +++
> src/pacman/pacman.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++-
> 6 files changed, 137 insertions(+), 118 deletions(-)
>
> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> index 7b7ca4e..f444e93 100644
> --- a/lib/libalpm/alpm.h
> +++ b/lib/libalpm/alpm.h
> @@ -83,6 +83,17 @@ int alpm_logaction(char *fmt, ...);
> typedef void (*alpm_cb_download)(const char *filename,
> off_t xfered, off_t total);
> typedef void (*alpm_cb_totaldl)(off_t total);
> +/** A callback for downloading files
> + * @param url the URL of the file to be downloaded
> + * @param localpath the directory to which the file should be downloaded
> + * @param mtimeold the modification time of the file previously downloaded
> + * @param mtimenew the modification time of the newly downloaded file.
> + * This should be set by the callback.
> + * @return 0 on success, 1 if the modification times are identical, -1 on
> + * error.
> + */
> +typedef int (*alpm_cb_fetch)(const char *url, const char *localpath,
> + time_t mtimeold, time_t *mtimenew);
Does everyone thing this is a good enough interface? I don't want to
have to change this later after we release once.
>
> /*
> * Options
> @@ -94,6 +105,9 @@ void alpm_option_set_logcb(alpm_cb_log cb);
> alpm_cb_download alpm_option_get_dlcb();
> void alpm_option_set_dlcb(alpm_cb_download cb);
>
> +alpm_cb_fetch alpm_option_get_fetchcb();
> +void alpm_option_set_fetchcb(alpm_cb_fetch cb);
> +
> alpm_cb_totaldl alpm_option_get_totaldlcb();
> void alpm_option_set_totaldlcb(alpm_cb_totaldl cb);
>
> @@ -137,9 +151,6 @@ void alpm_option_add_ignoregrp(const char *grp);
> void alpm_option_set_ignoregrps(alpm_list_t *ignoregrps);
> int alpm_option_remove_ignoregrp(const char *grp);
>
> -const char *alpm_option_get_xfercommand();
> -void alpm_option_set_xfercommand(const char *cmd);
> -
> unsigned short alpm_option_get_nopassiveftp();
> void alpm_option_set_nopassiveftp(unsigned short nopasv);
> void alpm_option_set_usedelta(unsigned short usedelta);
> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index 5b0a691..bc36706 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -262,111 +262,16 @@ cleanup:
> }
> #endif
>
> -static int download_external(const char *url, const char *localpath,
> - time_t mtimeold, time_t *mtimenew) {
> - int ret = 0;
> - int retval;
> - int usepart = 0;
> - char *ptr1, *ptr2;
> - char origCmd[PATH_MAX];
> - char parsedCmd[PATH_MAX] = "";
> - char cwd[PATH_MAX];
> - char *destfile, *tempfile, *filename;
> -
> - if(!handle->xfercommand) {
> - RET_ERR(PM_ERR_EXTERNAL_DOWNLOAD, -1);
> - }
> -
> - filename = get_filename(url);
> - if(!filename) {
> - RET_ERR(PM_ERR_EXTERNAL_DOWNLOAD, -1);
> - }
> - destfile = get_destfile(localpath, filename);
> - tempfile = get_tempfile(localpath, filename);
> -
> - /* replace all occurrences of %o with fn.part */
> - strncpy(origCmd, handle->xfercommand, sizeof(origCmd));
> - ptr1 = origCmd;
> - while((ptr2 = strstr(ptr1, "%o"))) {
> - usepart = 1;
> - ptr2[0] = '\0';
> - strcat(parsedCmd, ptr1);
> - strcat(parsedCmd, tempfile);
> - ptr1 = ptr2 + 2;
> - }
> - strcat(parsedCmd, ptr1);
> - /* replace all occurrences of %u with the download URL */
> - strncpy(origCmd, parsedCmd, sizeof(origCmd));
> - parsedCmd[0] = '\0';
> - ptr1 = origCmd;
> - while((ptr2 = strstr(ptr1, "%u"))) {
> - ptr2[0] = '\0';
> - strcat(parsedCmd, ptr1);
> - strcat(parsedCmd, url);
> - ptr1 = ptr2 + 2;
> - }
> - strcat(parsedCmd, ptr1);
> - /* cwd to the download directory */
> - getcwd(cwd, PATH_MAX);
> - if(chdir(localpath)) {
> - _alpm_log(PM_LOG_WARNING, _("could not chdir to %s\n"), localpath);
> - pm_errno = PM_ERR_EXTERNAL_DOWNLOAD;
> - ret = -1;
> - goto cleanup;
> - }
> - /* execute the parsed command via /bin/sh -c */
> - _alpm_log(PM_LOG_DEBUG, "running command: %s\n", parsedCmd);
> - retval = system(parsedCmd);
> -
> - if(retval == -1) {
> - _alpm_log(PM_LOG_WARNING, _("running XferCommand: fork failed!\n"));
> - pm_errno = PM_ERR_EXTERNAL_DOWNLOAD;
> - ret = -1;
> - } else if(retval != 0) {
> - /* download failed */
> - _alpm_log(PM_LOG_DEBUG, "XferCommand command returned non-zero status "
> - "code (%d)\n", retval);
> - ret = -1;
> - } else {
> - /* download was successful */
> - if(usepart) {
> - rename(tempfile, destfile);
> - }
> - ret = 0;
> - }
> -
> -cleanup:
> - chdir(cwd);
> - if(ret == -1) {
> - /* hack to let an user the time to cancel a download */
> - sleep(2);
> - }
> - FREE(destfile);
> - FREE(tempfile);
> -
> - return(ret);
> -}
> -
> static int download(const char *url, const char *localpath,
> time_t mtimeold, time_t *mtimenew) {
> - int ret;
> -
> - /* We have a few things to take into account here.
> - * 1. If we have both internal/external available, choose based on
> - * whether xfercommand is populated.
> - * 2. If we only have external available, we should first check
> - * if a command was provided before we drop into download_external.
> - */
> - if(handle->xfercommand == NULL) {
> + if(handle->fetchcb == NULL) {
> #if defined(INTERNAL_DOWNLOAD)
> - ret = download_internal(url, localpath, mtimeold, mtimenew);
> + handle->fetchcb = download_internal;
> #else
> RET_ERR(PM_ERR_EXTERNAL_DOWNLOAD, -1);
> #endif
> - } else {
> - ret = download_external(url, localpath, mtimeold, mtimenew);
> }
> - return(ret);
> + return handle->fetchcb(url, localpath, mtimeold, mtimeold);
> }
>
> /*
> diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c
> index 813f439..3566889 100644
> --- a/lib/libalpm/handle.c
> +++ b/lib/libalpm/handle.c
> @@ -105,6 +105,15 @@ alpm_cb_download SYMEXPORT alpm_option_get_dlcb()
> return handle->dlcb;
> }
>
> +alpm_cb_fetch SYMEXPORT alpm_option_get_fetchcb()
> +{
> + if (handle == NULL) {
> + pm_errno = PM_ERR_HANDLE_NULL;
> + return NULL;
> + }
> + return handle->fetchcb;
> +}
> +
> alpm_cb_totaldl SYMEXPORT alpm_option_get_totaldlcb()
> {
> if (handle == NULL) {
> @@ -204,15 +213,6 @@ alpm_list_t SYMEXPORT *alpm_option_get_ignoregrps()
> return handle->ignoregrp;
> }
>
> -const char SYMEXPORT *alpm_option_get_xfercommand()
> -{
> - if (handle == NULL) {
> - pm_errno = PM_ERR_HANDLE_NULL;
> - return NULL;
> - }
> - return handle->xfercommand;
> -}
> -
> unsigned short SYMEXPORT alpm_option_get_nopassiveftp()
> {
> if (handle == NULL) {
> @@ -258,6 +258,15 @@ void SYMEXPORT alpm_option_set_dlcb(alpm_cb_download cb)
> handle->dlcb = cb;
> }
>
> +void SYMEXPORT alpm_option_set_fetchcb(alpm_cb_fetch cb)
> +{
> + if (handle == NULL) {
> + pm_errno = PM_ERR_HANDLE_NULL;
> + return;
> + }
> + handle->fetchcb = cb;
> +}
> +
> void SYMEXPORT alpm_option_set_totaldlcb(alpm_cb_totaldl cb)
> {
> if (handle == NULL) {
> @@ -519,12 +528,6 @@ int SYMEXPORT alpm_option_remove_ignoregrp(const char *grp)
> return(0);
> }
>
> -void SYMEXPORT alpm_option_set_xfercommand(const char *cmd)
> -{
> - if(handle->xfercommand) FREE(handle->xfercommand);
> - if(cmd) handle->xfercommand = strdup(cmd);
> -}
> -
> void SYMEXPORT alpm_option_set_nopassiveftp(unsigned short nopasv)
> {
> handle->nopassiveftp = nopasv;
> diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h
> index ad7666d..89fc457 100644
> --- a/lib/libalpm/handle.h
> +++ b/lib/libalpm/handle.h
> @@ -40,6 +40,7 @@ typedef struct _pmhandle_t {
> alpm_cb_log logcb; /* Log callback function */
> alpm_cb_download dlcb; /* Download callback function */
> alpm_cb_totaldl totaldlcb; /* Total download callback function */
> + alpm_cb_fetch fetchcb; /* Download file callback function */
>
> /* filesystem paths */
> char *root; /* Root path, default '/' */
> @@ -57,7 +58,6 @@ typedef struct _pmhandle_t {
> /* options */
> unsigned short usesyslog; /* Use syslog instead of logfile? */ /* TODO move to frontend */
> unsigned short nopassiveftp; /* Don't use PASV ftp connections */
> - char *xfercommand; /* External download command */
> unsigned short usedelta; /* Download deltas if possible */
> } pmhandle_t;
>
> diff --git a/src/pacman/conf.h b/src/pacman/conf.h
> index 466d983..650b0f9 100644
> --- a/src/pacman/conf.h
> +++ b/src/pacman/conf.h
> @@ -20,6 +20,11 @@
> #define _PM_CONF_H
>
> #include <alpm.h>
> +#if defined(HAVE_SYS_SYSLIMITS_H)
> +#include <sys/syslimits.h> /* PATH_MAX */
> +#else
> +#define PATH_MAX 1024
> +#endif
>
> typedef struct __config_t {
> unsigned short op;
> @@ -71,6 +76,7 @@ typedef struct __config_t {
> unsigned short cleanmethod; /* select -Sc behavior */
> alpm_list_t *holdpkg;
> alpm_list_t *syncfirst;
> + char xfercommand[PATH_MAX]; /* external download command */
No. Use malloc like everything else, please.
> } config_t;
>
> /* Operations */
> diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c
> index 59916d6..0d6c897 100644
> --- a/src/pacman/pacman.c
> +++ b/src/pacman/pacman.c
> @@ -573,6 +573,99 @@ static void setrepeatingoption(const char *ptr, const char *option,
> pm_printf(PM_LOG_DEBUG, "config: %s: %s\n", option, p);
> }
>
> +/** External fetch callback */
> +static int _download_with_xfercommand(const char *url, const char *localpath,
> + time_t mtimeold, time_t *mtimenew) {
Kill the underscore in the function name- the only reason
_parseconfig() has it is because it is an internal loop of
parseconfig(). In addition, I don't think you can make this static as
we are attempting to pass a function pointer to the backend, and
static could potentially cause it to be dropped and/or inlined.
I'm assuming the below code didn't really change from what was in the
backend- can you verify this, or point out what you did modify?
> + int ret = 0;
> + int retval;
> + int usepart = 0;
> + char *ptr1, *ptr2;
> + char origCmd[PATH_MAX];
> + char parsedCmd[PATH_MAX] = "";
> + char cwd[PATH_MAX];
> + char *destfile, *tempfile, *filename;
> + int len;
> +
> + if(!config->xfercommand) {
> + return -1;
> + }
> +
> + filename = strrchr(url, '/');
> + if(!filename) {
> + return -1;
> + } else {
> + filename++; /* omit leading slash */
> + }
> +
> + len = strlen(localpath) + strlen(filename) + 1;
> + destfile = calloc(len, sizeof(*destfile));
> + snprintf(destfile, len, "%s%s", localpath, filename);
> +
> + len += 5; /* ".part" */
> + tempfile = calloc(len, sizeof(*tempfile));
> + snprintf(tempfile, len, "%s.part", destfile);
> +
> + strncpy(origCmd, config->xfercommand, sizeof(origCmd));
> + /* replace all occurrences of %o with fn.part */
> + ptr1 = origCmd;
> + while((ptr2 = strstr(ptr1, "%o"))) {
> + usepart = 1;
> + ptr2[0] = '\0';
> + strcat(parsedCmd, ptr1);
> + strcat(parsedCmd, tempfile);
> + ptr1 = ptr2 + 2;
> + }
> + strcat(parsedCmd, ptr1);
> + /* replace all occurrences of %u with the download URL */
> + strncpy(origCmd, parsedCmd, sizeof(origCmd));
> + parsedCmd[0] = '\0';
> + ptr1 = origCmd;
> + while((ptr2 = strstr(ptr1, "%u"))) {
> + ptr2[0] = '\0';
> + strcat(parsedCmd, ptr1);
> + strcat(parsedCmd, url);
> + ptr1 = ptr2 + 2;
> + }
> + strcat(parsedCmd, ptr1);
> + /* cwd to the download directory */
> + getcwd(cwd, PATH_MAX);
> + if(chdir(localpath)) {
> + pm_printf(PM_LOG_DEBUG, "could not chdir to %s\n", localpath);
> + ret = -1;
> + goto cleanup;
> + }
> + /* execute the parsed command via /bin/sh -c */
> + pm_printf(PM_LOG_DEBUG, "running command: %s\n", parsedCmd);
> + retval = system(parsedCmd);
> +
> + if(retval == -1) {
> + pm_printf(PM_LOG_DEBUG, "running XferCommand: fork failed!\n");
> + ret = -1;
> + } else if(retval != 0) {
> + /* download failed */
> + pm_printf(PM_LOG_DEBUG, "XferCommand command returned non-zero status "
> + "code (%d)\n", retval);
> + ret = -1;
> + } else {
> + /* download was successful */
> + if(usepart) {
> + rename(tempfile, destfile);
> + }
> + ret = 0;
> + }
> +
> +cleanup:
> + chdir(cwd);
> + if(ret == -1) {
> + /* hack to let an user the time to cancel a download */
> + sleep(2);
> + }
> + free(destfile);
> + free(tempfile);
> +
> + return(ret);
> +}
> +
> /* The real parseconfig. Called with a null section argument by the publicly
> * visible parseconfig so we can recall from within ourself on an include */
> static int _parseconfig(const char *file, const char *givensection,
> @@ -736,7 +829,8 @@ static int _parseconfig(const char *file, const char *givensection,
> pm_printf(PM_LOG_DEBUG, "config: logfile: %s\n", ptr);
> }
> } else if (strcmp(key, "XferCommand") == 0) {
> - alpm_option_set_xfercommand(ptr);
> + strncpy(config->xfercommand, ptr, sizeof(config->xfercommand));
> + alpm_option_set_fetchcb(_download_with_xfercommand);
You probably want to make this more like the RootDir or LogFile config
above it and do the dynamic allocation.
> pm_printf(PM_LOG_DEBUG, "config: xfercommand: %s\n", ptr);
> } else if (strcmp(key, "CleanMethod") == 0) {
> if (strcmp(ptr, "KeepInstalled") == 0) {
More information about the pacman-dev
mailing list