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 when if it is defined, otherwise the old behavior applies.
Signed-off-by: Sebastian Nowicki sebnow@gmail.com --- lib/libalpm/alpm.h | 14 ++++++++++++++ lib/libalpm/dload.c | 9 ++++++--- lib/libalpm/handle.c | 18 ++++++++++++++++++ lib/libalpm/handle.h | 1 + 4 files changed, 39 insertions(+), 3 deletions(-)
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 7b7ca4e..266f7ff 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);
/* * 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);
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 5b0a691..a244999 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -352,12 +352,15 @@ static int download(const char *url, const char *localpath, int ret;
/* We have a few things to take into account here. - * 1. If we have both internal/external available, choose based on + * 1. If the fetch callback is set, prefer it. + * 2. 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 + * 3. 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) { + ret = handle->fetchcb(url, localpath, mtimeold, mtimenew); + } else if(handle->xfercommand == NULL) { #if defined(INTERNAL_DOWNLOAD) ret = download_internal(url, localpath, mtimeold, mtimenew); #else diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 813f439..f19bc3f 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) { @@ -258,6 +267,15 @@ void SYMEXPORT alpm_option_set_dlcb(alpm_cb_download cb) handle->dlcb = cb; }
+void 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) { diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index ad7666d..f895d85 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 '/' */
On Thu, Feb 19, 2009 at 12:55 AM, Sebastian Nowicki sebnow@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 when if it is defined, otherwise the old behavior applies.
I like this, but my first thought is: why not extrapolate our current downloading as a "fetch callback" too? That way the check just needs a "!= NULL" before calling it, and it can be done on config parsing:
//in initialization #ifdef INTERNAL_DOWNLOAD fetchcb = internal_fetch; #else fetchcb = NULL; #endif ... ... // in config parsing if(strcmp(blah, "XferCommand") == 0) { fetchcb = use_xfercommand; }
On 20/02/2009, at 2:03 AM, Aaron Griffin wrote:
On Thu, Feb 19, 2009 at 12:55 AM, Sebastian Nowicki sebnow@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 when if it is defined, otherwise the old behavior applies.
Just noticed it should be s/when//.
I like this, but my first thought is: why not extrapolate our current downloading as a "fetch callback" too? That way the check just needs a "!= NULL" before calling it, and it can be done on config parsing:
I had the same idea, but decided to leave it as is so that the two methods can be distinguished. Re-thinking it, they don't really need to be distinguished (in alpm), since the callback is used if the front- end specifies it either way, and the default could be the internal download function. It's quite late atm so I won't send the patch just now ;).
//in initialization #ifdef INTERNAL_DOWNLOAD fetchcb = internal_fetch; #else fetchcb = NULL; #endif ... ... // in config parsing if(strcmp(blah, "XferCommand") == 0) { fetchcb = use_xfercommand; }
Indeed, much simpler. It seems this makes download() unecessary as it would only call handle->fetchcb(), and as far as I can tell it only gets called in _alpm_download_single_file() and alpm_fetch_pkgurl().
On Thu, Feb 19, 2009 at 1:11 PM, Sebastian Nowicki sebnow@gmail.com wrote:
On 20/02/2009, at 2:03 AM, Aaron Griffin wrote:
On Thu, Feb 19, 2009 at 12:55 AM, Sebastian Nowicki sebnow@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 when if it is defined, otherwise the old behavior applies.
Just noticed it should be s/when//.
I like this, but my first thought is: why not extrapolate our current downloading as a "fetch callback" too? That way the check just needs a "!= NULL" before calling it, and it can be done on config parsing:
I had the same idea, but decided to leave it as is so that the two methods can be distinguished. Re-thinking it, they don't really need to be distinguished (in alpm), since the callback is used if the front-end specifies it either way, and the default could be the internal download function. It's quite late atm so I won't send the patch just now ;).
//in initialization #ifdef INTERNAL_DOWNLOAD fetchcb = internal_fetch; #else fetchcb = NULL; #endif ... ... // in config parsing if(strcmp(blah, "XferCommand") == 0) { fetchcb = use_xfercommand; }
Indeed, much simpler. It seems this makes download() unecessary as it would only call handle->fetchcb(), and as far as I can tell it only gets called in _alpm_download_single_file() and alpm_fetch_pkgurl().
Yeah, we'd just need to make sure that there is a "if fetchcb == NULL) { error and return 1 }" We don't want to fail early on this, as a download function isn't always needed
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.
Signed-off-by: Sebastian Nowicki sebnow@gmail.com --- lib/libalpm/alpm.h | 14 +++++++ lib/libalpm/dload.c | 101 +------------------------------------------------ lib/libalpm/handle.c | 18 +++++++++ lib/libalpm/handle.h | 1 + src/pacman/conf.h | 6 +++ src/pacman/pacman.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++- 6 files changed, 137 insertions(+), 99 deletions(-)
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 7b7ca4e..266f7ff 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);
/* * 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);
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..84e2c3f 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) { @@ -258,6 +267,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) { diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index ad7666d..f895d85 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 '/' */ 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 */ } 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) { + 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); pm_printf(PM_LOG_DEBUG, "config: xfercommand: %s\n", ptr); } else if (strcmp(key, "CleanMethod") == 0) { if (strcmp(ptr, "KeepInstalled") == 0) {
I left the download() function to lazily initialize handle->fetchcb, since it couldn't be done from _alpm_handle_new().
On Fri, Feb 20, 2009 at 4:21 PM, Sebastian Nowicki sebnow@gmail.com wrote:
The external download method was moved to pacman and is set as the fetch callback, if the command is defined in the configuration file.
I'm not sure if this is what you meant. It seems like a bit change (could break front-end code), but I didn't see any other way. I just noticed I forgot to remove the xfercommand related stuff from libalpm. Re-sending patch in a second.
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@gmail.com --- 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);
/* * 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 */ } 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) { + 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); pm_printf(PM_LOG_DEBUG, "config: xfercommand: %s\n", ptr); } else if (strcmp(key, "CleanMethod") == 0) { if (strcmp(ptr, "KeepInstalled") == 0) {
Hello,
I'm all in for this, thanks Sebastian. This would allow to remove a _HUGE_ quantity of unneeded callbacks in alpm, and from my point of view, it is really great (especially for Shaman/Aqpm). For what it's worth, I'd like to add my vote here and I really hope this patch will be in trunk soon, so that I can switch Shaman's frontend to this new system.
Dario
In data venerdì 20 febbraio 2009 08:31:07, Sebastian Nowicki ha scritto: : > 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@gmail.com
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);/*
- 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 */
} 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) {
- 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); pm_printf(PM_LOG_DEBUG, "config: xfercommand: %s\n", ptr); } else if (strcmp(key, "CleanMethod") == 0) { if (strcmp(ptr, "KeepInstalled") == 0) {
On Fri, Feb 20, 2009 at 8:34 AM, Dario Freddi drf54321@gmail.com wrote:
Hello,
I'm all in for this, thanks Sebastian. This would allow to remove a _HUGE_ quantity of unneeded callbacks in alpm, and from my point of view, it is really great (especially for Shaman/Aqpm). For what it's worth, I'd like to add my vote here and I really hope this patch will be in trunk soon, so that I can switch Shaman's frontend to this new system.
Agreed - that's why I thought it was a good idea :)
Bump. Any changes required? I might be offline for a while soon, so I'd just like to get an OK on it.
Bumping again. Any chances to get this into the main tree?
In data giovedì 05 marzo 2009 07:02:50, Sebastian Nowicki ha scritto: : > Bump. Any changes required? I might be offline for a while soon, so
I'd just like to get an OK on it.
pacman-dev mailing list pacman-dev@archlinux.org http://www.archlinux.org/mailman/listinfo/pacman-dev
On Tue, Mar 10, 2009 at 6:26 PM, Dario Freddi drf54321@gmail.com wrote:
Bumping again. Any chances to get this into the main tree?
P-p-p-please, Dan? *puppy dog eyes*
Not really a bump. I just wanted to know if this patch will ever be in, no matter when.
To talk bluntly: if you assure me this patch is going to make it in the main pacman tree, and you simply don't know when, I'll apply it in my local copy and start hacking with the new download api.
I just want to avoid me some hours of useless coding. So I'd just like to know if this patch will ever make it in.
On Thursday 05 March 2009 07:02:50 Sebastian Nowicki wrote:
Bump. Any changes required? I might be offline for a while soon, so I'd just like to get an OK on it.
pacman-dev mailing list pacman-dev@archlinux.org http://www.archlinux.org/mailman/listinfo/pacman-dev
2009/3/27 Dario Freddi drf54321@gmail.com:
Not really a bump. I just wanted to know if this patch will ever be in, no matter when.
To talk bluntly: if you assure me this patch is going to make it in the main pacman tree, and you simply don't know when, I'll apply it in my local copy and start hacking with the new download api.
I just want to avoid me some hours of useless coding. So I'd just like to know if this patch will ever make it in.
I hope it does. Dan has been very busy recently, but he is the authority on pacman these days, not myself. The best I can do is give my little seal of approval on this (FWIW).
Any word, guys? Dan? Xavier? Nagy?
2009/3/27 Dario Freddi drf54321@gmail.com:
Not really a bump. I just wanted to know if this patch will ever be in, no matter when.
To talk bluntly: if you assure me this patch is going to make it in the main pacman tree, and you simply don't know when, I'll apply it in my local copy and start hacking with the new download api.
I just want to avoid me some hours of useless coding. So I'd just like to know if this patch will ever make it in.
I hope it does. Dan has been very busy recently, but he is the authority on pacman these days, not myself. The best I can do is give my little seal of approval on this (FWIW).
Any word, guys? Dan? Xavier? Nagy?
I haven't reviewed this patch rigorously, but I like the concept.
Bye
On Fri, Mar 27, 2009 at 2:57 PM, Nagy Gabor ngaba@bibl.u-szeged.hu wrote:
2009/3/27 Dario Freddi drf54321@gmail.com:
Not really a bump. I just wanted to know if this patch will ever be in, no matter when.
To talk bluntly: if you assure me this patch is going to make it in the main pacman tree, and you simply don't know when, I'll apply it in my local copy and start hacking with the new download api.
I just want to avoid me some hours of useless coding. So I'd just like to know if this patch will ever make it in.
I hope it does. Dan has been very busy recently, but he is the authority on pacman these days, not myself. The best I can do is give my little seal of approval on this (FWIW).
Any word, guys? Dan? Xavier? Nagy?
I haven't reviewed this patch rigorously, but I like the concept.
Sorry, man. I meant to look at this a long time ago but I have been quite busy lately.
I definitely like the concept, I just haven't had a chance to review it. I would be fairly certain this patch will go in at some point, although I might suggest a few changes.
Hopefully I will look closer this weekend- sorry again.
-Dan
On Saturday 28 March 2009 01:13:04 Dan McGee wrote:
Sorry, man. I meant to look at this a long time ago but I have been quite busy lately.
I definitely like the concept, I just haven't had a chance to review it. I would be fairly certain this patch will go in at some point, although I might suggest a few changes.
Hopefully I will look closer this weekend- sorry again.
No problem Dan: as I told you, I don't blame you for being busy or whatever, since I'm the first who can disappear for months due to things happening in my life.
All I wanted to know was if the concept itself was good for you guys: I'll start working on this today. Also, when the patch will get in, maybe I'll try adding support for multisource and/or concurrent downloading, depending on what the final patch will look like.
-Dan _______________________________________________ pacman-dev mailing list pacman-dev@archlinux.org http://www.archlinux.org/mailman/listinfo/pacman-dev
On Fri, Feb 20, 2009 at 2:31 AM, Sebastian Nowicki sebnow@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@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) {
On Sat, Mar 28, 2009 at 4:34 PM, Dan McGee dpmcgee@gmail.com wrote:
On Fri, Feb 20, 2009 at 2:31 AM, Sebastian Nowicki sebnow@gmail.com wrote:
+/** 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.
Hmmm I wonder... why do we need to pass the mtime stuff in? Doesn't it make sense that most fetch callbacks would do the exact same comparison? I can't see any other need for these - except maybe mtimenew to set the download file's mtime to match the server's, but... do we care that they are exact? If we don't set it, it should always be greater than the server anyway.
I'm kinda just talking here, not putting lots of thought into it, but are those two points (mtime compare, and setting local file mtime) sane? Is there something I'm missing?
On Sat, Mar 28, 2009 at 4:44 PM, Aaron Griffin aaronmgriffin@gmail.com wrote:
On Sat, Mar 28, 2009 at 4:34 PM, Dan McGee dpmcgee@gmail.com wrote:
On Fri, Feb 20, 2009 at 2:31 AM, Sebastian Nowicki sebnow@gmail.com wrote:
+/** 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.
Hmmm I wonder... why do we need to pass the mtime stuff in? Doesn't it make sense that most fetch callbacks would do the exact same comparison? I can't see any other need for these - except maybe mtimenew to set the download file's mtime to match the server's, but... do we care that they are exact? If we don't set it, it should always be greater than the server anyway.
I'm kinda just talking here, not putting lots of thought into it, but are those two points (mtime compare, and setting local file mtime) sane? Is there something I'm missing?
The mtime stuff is mainly used for database downloads, where we don't actually have the old file to compare against- we simply have the mtime stored in: $ cat /var/lib/pacman/sync/core/.lastupdate 1238199239
We could probably simplify this by always keeping the db file we download around and just using the stat-ed mtime from that...
-Dan
On Sat, Mar 28, 2009 at 4:47 PM, Dan McGee dpmcgee@gmail.com wrote:
On Sat, Mar 28, 2009 at 4:44 PM, Aaron Griffin aaronmgriffin@gmail.com wrote:
On Sat, Mar 28, 2009 at 4:34 PM, Dan McGee dpmcgee@gmail.com wrote:
On Fri, Feb 20, 2009 at 2:31 AM, Sebastian Nowicki sebnow@gmail.com wrote:
+/** 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.
Hmmm I wonder... why do we need to pass the mtime stuff in? Doesn't it make sense that most fetch callbacks would do the exact same comparison? I can't see any other need for these - except maybe mtimenew to set the download file's mtime to match the server's, but... do we care that they are exact? If we don't set it, it should always be greater than the server anyway.
I'm kinda just talking here, not putting lots of thought into it, but are those two points (mtime compare, and setting local file mtime) sane? Is there something I'm missing?
The mtime stuff is mainly used for database downloads, where we don't actually have the old file to compare against- we simply have the mtime stored in: $ cat /var/lib/pacman/sync/core/.lastupdate 1238199239
We could probably simplify this by always keeping the db file we download around and just using the stat-ed mtime from that...
What about the compare? if(mtimenew > mtimeold) will be done in every fetch callback, correct? Why don't e just do that before we call the fetch callback?
On Sat, Mar 28, 2009 at 4:51 PM, Aaron Griffin aaronmgriffin@gmail.com wrote:
On Sat, Mar 28, 2009 at 4:47 PM, Dan McGee dpmcgee@gmail.com wrote:
On Sat, Mar 28, 2009 at 4:44 PM, Aaron Griffin aaronmgriffin@gmail.com wrote:
On Sat, Mar 28, 2009 at 4:34 PM, Dan McGee dpmcgee@gmail.com wrote:
On Fri, Feb 20, 2009 at 2:31 AM, Sebastian Nowicki sebnow@gmail.com wrote:
+/** 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.
Hmmm I wonder... why do we need to pass the mtime stuff in? Doesn't it make sense that most fetch callbacks would do the exact same comparison? I can't see any other need for these - except maybe mtimenew to set the download file's mtime to match the server's, but... do we care that they are exact? If we don't set it, it should always be greater than the server anyway.
I'm kinda just talking here, not putting lots of thought into it, but are those two points (mtime compare, and setting local file mtime) sane? Is there something I'm missing?
The mtime stuff is mainly used for database downloads, where we don't actually have the old file to compare against- we simply have the mtime stored in: $ cat /var/lib/pacman/sync/core/.lastupdate 1238199239
We could probably simplify this by always keeping the db file we download around and just using the stat-ed mtime from that...
What about the compare? if(mtimenew > mtimeold) will be done in every fetch callback, correct? Why don't e just do that before we call the fetch callback?
Because we don't have mtimenew- thats a pointer being passed in that gets filled by the method so we can use the value when we come back from the download callback. The fetch method has to do something (such as a HEAD request) first before the compare can be done.
-Dan
On Saturday 28 March 2009 23:06:07 Dan McGee wrote:
Because we don't have mtimenew- thats a pointer being passed in that gets filled by the method so we can use the value when we come back from the download callback. The fetch method has to do something (such as a HEAD request) first before the compare can be done.
Exactly as Dan says. In my first implementation I added a head retrival first to check & store the last modification time of the file I'm trying to download, and if the times don't match, I proceed with the download.
Uhm, I also wonder: a better implementation (but I know it's harder) would be having a callback that sends a list files to download (maybe pack all needed information in a struct and send a linked list), so that frontends could implement some better download methods (like apt does, for example). Then libalpm would have a callback to proceed with the operation. I could give a try and implement it, what do you think?
-Dan _______________________________________________ pacman-dev mailing list pacman-dev@archlinux.org http://www.archlinux.org/mailman/listinfo/pacman-dev
On 29/03/2009, at 5:34 AM, Dan McGee wrote:
On Fri, Feb 20, 2009 at 2:31 AM, Sebastian Nowicki sebnow@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@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
I compiled it and tested. I must have not compiled with --enable-debug or something. Perhaps my last change broke it (I might have forgotten to test, oops), which is more likely. Sorry either way.
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?
For the most part, yes. I did "inline" some functions, which were static. There might have been other minor changes.
Quickly looking over the log, I changed this:
filename = get_filename(url); if(!filename) { RET_ERR(PM_ERR_EXTERNAL_DOWNLOAD, -1); } destfile = get_destfile(localpath, filename); tempfile = get_tempfile(localpath, filename);
To this:
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);
I'll fix the patch up tomorrow.
On Fri, Apr 3, 2009 at 11:20 AM, Sebastian Nowicki sebnow@gmail.com wrote:
On 29/03/2009, at 5:34 AM, Dan McGee wrote:
On Fri, Feb 20, 2009 at 2:31 AM, Sebastian Nowicki sebnow@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@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
I compiled it and tested. I must have not compiled with --enable-debug or something. Perhaps my last change broke it (I might have forgotten to test, oops), which is more likely. Sorry either way.
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?
For the most part, yes. I did "inline" some functions, which were static. There might have been other minor changes.
Was this strictly necessary? I much prefer things being split out in functions as it is much easier to read. gcc will *always* optimize these back in if they are called as infrequently as they probably are (or in a tight loop), so don't try to outsmart the compiler.
I'll try to do a comparison diff sometime to see what changed once you resubmit the patch.
Quickly looking over the log, I changed this:
filename = get_filename(url); if(!filename) { RET_ERR(PM_ERR_EXTERNAL_DOWNLOAD, -1); } destfile = get_destfile(localpath, filename); tempfile = get_tempfile(localpath, filename);
To this:
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);
I'll fix the patch up tomorrow. _______________________________________________ pacman-dev mailing list pacman-dev@archlinux.org http://www.archlinux.org/mailman/listinfo/pacman-dev
On 04/04/2009, at 1:48 AM, Dan McGee wrote:
On Fri, Apr 3, 2009 at 11:20 AM, Sebastian Nowicki sebnow@gmail.com wrote:
On 29/03/2009, at 5:34 AM, Dan McGee wrote:
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?
For the most part, yes. I did "inline" some functions, which were static. There might have been other minor changes.
Was this strictly necessary? I much prefer things being split out in functions as it is much easier to read. gcc will *always* optimize these back in if they are called as infrequently as they probably are (or in a tight loop), so don't try to outsmart the compiler.
There were two choices, inlining it like I did, or copying and pasting the functions. Since they were used only once (as far as I know), I chose manual inlining. The functions are static within the libalpm code, so I couldn't have simply called them, unfortunately. This wasn't an optimisation of any sort.
I have just finished implementing this in aqpm and actually almost finished it thanks to this, so I'm freeing up resources to help you guys on this patch if needed.
On Saturday 04 April 2009 07:18:35 Sebastian Nowicki wrote:
On 04/04/2009, at 1:48 AM, Dan McGee wrote:
On Fri, Apr 3, 2009 at 11:20 AM, Sebastian Nowicki
sebnow@gmail.com wrote:
On 29/03/2009, at 5:34 AM, Dan McGee wrote:
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?
For the most part, yes. I did "inline" some functions, which were static. There might have been other minor changes.
Was this strictly necessary? I much prefer things being split out in functions as it is much easier to read. gcc will *always* optimize these back in if they are called as infrequently as they probably are (or in a tight loop), so don't try to outsmart the compiler.
There were two choices, inlining it like I did, or copying and pasting the functions. Since they were used only once (as far as I know), I chose manual inlining. The functions are static within the libalpm code, so I couldn't have simply called them, unfortunately. This wasn't an optimisation of any sort.
pacman-dev mailing list pacman-dev@archlinux.org http://www.archlinux.org/mailman/listinfo/pacman-dev
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 when if it is defined, otherwise the old behavior applies.
Signed-off-by: Sebastian Nowicki sebnow@gmail.com --- lib/libalpm/alpm.h | 14 ++++++ lib/libalpm/dload.c | 101 +------------------------------------------ lib/libalpm/handle.c | 34 ++++++++------- lib/libalpm/handle.h | 2 +- src/pacman/conf.c | 1 + src/pacman/conf.h | 1 + src/pacman/pacman.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++- 7 files changed, 152 insertions(+), 116 deletions(-)
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 43df31c..4eb99b6 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);
/* * 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);
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 5b0a691..eeaf476 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, mtimenew); }
/* diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 813f439..d1542cf 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -78,7 +78,6 @@ void _alpm_handle_free(pmhandle_t *handle) FREELIST(handle->cachedirs); FREE(handle->logfile); FREE(handle->lockfile); - FREE(handle->xfercommand); FREELIST(handle->dbs_sync); FREELIST(handle->noupgrade); FREELIST(handle->noextract); @@ -105,6 +104,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 +212,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 +257,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 +527,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.c b/src/pacman/conf.c index ca5cd12..ae4e77d 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -60,6 +60,7 @@ int config_free(config_t *oldconfig) free(oldconfig->rootdir); free(oldconfig->dbpath); free(oldconfig->logfile); + free(oldconfig->xfercommand); free(oldconfig); oldconfig = NULL;
diff --git a/src/pacman/conf.h b/src/pacman/conf.h index 466d983..5e1d933 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -71,6 +71,7 @@ typedef struct __config_t { unsigned short cleanmethod; /* select -Sc behavior */ alpm_list_t *holdpkg; alpm_list_t *syncfirst; + char *xfercommand; /* external download command */ } config_t;
/* Operations */ diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 59916d6..0071d35 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -573,6 +573,118 @@ static void setrepeatingoption(const char *ptr, const char *option, pm_printf(PM_LOG_DEBUG, "config: %s: %s\n", option, p); }
+static char *get_filename(const char *url) { + char *filename = strrchr(url, '/'); + if(filename != NULL) { + filename++; + } + return(filename); +} + +static char *get_destfile(const char *path, const char *filename) { + char *destfile; + /* len = localpath len + filename len + null */ + int len = strlen(path) + strlen(filename) + 1; + destfile = calloc(len, sizeof(char)); + snprintf(destfile, len, "%s%s", path, filename); + + return(destfile); +} + +static char *get_tempfile(const char *path, const char *filename) { + char *tempfile; + /* len = localpath len + filename len + '.part' len + null */ + int len = strlen(path) + strlen(filename) + 6; + tempfile = calloc(len, sizeof(char)); + snprintf(tempfile, len, "%s%s.part", path, filename); + + return(tempfile); +} + +/** External fetch callback */ +int download_with_xfercommand(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(!config->xfercommand) { + return -1; + } + + filename = get_filename(url); + if(!filename) { + return -1; + } + destfile = get_destfile(localpath, filename); + tempfile = get_tempfile(localpath, filename); + + 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 +848,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); + config->xfercommand = strdup(ptr); + alpm_option_set_fetchcb(download_with_xfercommand); pm_printf(PM_LOG_DEBUG, "config: xfercommand: %s\n", ptr); } else if (strcmp(key, "CleanMethod") == 0) { if (strcmp(ptr, "KeepInstalled") == 0) {
I fixed the warnings and errors and tested it a bit more. Everything seems to be working fine. Using dynamic allocation for the xfercommand string removed the problem with PATH_MAX. The previously inlined functions are now separate functions as before, with minor changes due to the lack of macros from libalpm (see diff below).
On Sun, Mar 29, 2009 at 5:34 AM, Dan McGee dpmcgee@gmail.com wrote:
On Fri, Feb 20, 2009 at 2:31 AM, Sebastian Nowicki sebnow@gmail.com wrote:
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 wasn't sure about that either, but it worked as static. I changed it to non-static to be on the safe side.
On Sat, Apr 4, 2009 at 1:48 AM, Dan McGee dpmcgee@gmail.com wrote:
I'll try to do a comparison diff sometime to see what changed once you resubmit the patch.
Here's a diff of the functions moved over:
--- a 2009-04-04 16:13:57.000000000 +0800 +++ b 2009-04-04 16:14:45.000000000 +0800 @@ -10,7 +10,7 @@ static char *get_destfile(const char *pa char *destfile; /* len = localpath len + filename len + null */ int len = strlen(path) + strlen(filename) + 1; - CALLOC(destfile, len, sizeof(char), RET_ERR(PM_ERR_MEMORY, NULL)); + destfile = calloc(len, sizeof(char)); snprintf(destfile, len, "%s%s", path, filename);
return(destfile); @@ -20,13 +20,13 @@ static char *get_tempfile(const char *pa char *tempfile; /* len = localpath len + filename len + '.part' len + null */ int len = strlen(path) + strlen(filename) + 6; - CALLOC(tempfile, len, sizeof(char), RET_ERR(PM_ERR_MEMORY, NULL)); + tempfile = calloc(len, sizeof(char)); snprintf(tempfile, len, "%s%s.part", path, filename);
return(tempfile); }
-static int download_external(const char *url, const char *localpath, +int download_with_xfercommand(const char *url, const char *localpath, time_t mtimeold, time_t *mtimenew) { int ret = 0; int retval; @@ -37,19 +37,19 @@ static int download_external(const char char cwd[PATH_MAX]; char *destfile, *tempfile, *filename;
- if(!handle->xfercommand) { - RET_ERR(PM_ERR_EXTERNAL_DOWNLOAD, -1); + if(!config->xfercommand) { + return -1; }
filename = get_filename(url); if(!filename) { - RET_ERR(PM_ERR_EXTERNAL_DOWNLOAD, -1); + return -1; } destfile = get_destfile(localpath, filename); tempfile = get_tempfile(localpath, filename);
+ strncpy(origCmd, config->xfercommand, sizeof(origCmd)); /* replace all occurrences of %o with fn.part */ - strncpy(origCmd, handle->xfercommand, sizeof(origCmd)); ptr1 = origCmd; while((ptr2 = strstr(ptr1, "%o"))) { usepart = 1; @@ -73,22 +73,20 @@ static int download_external(const char /* 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; + pm_printf(PM_LOG_DEBUG, "could not chdir to %s\n", localpath); ret = -1; goto cleanup; } /* execute the parsed command via /bin/sh -c */ - _alpm_log(PM_LOG_DEBUG, "running command: %s\n", parsedCmd); + pm_printf(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; + pm_printf(PM_LOG_DEBUG, "running XferCommand: fork failed!\n"); ret = -1; } else if(retval != 0) { /* download failed */ - _alpm_log(PM_LOG_DEBUG, "XferCommand command returned non-zero status " + pm_printf(PM_LOG_DEBUG, "XferCommand command returned non-zero status " "code (%d)\n", retval); ret = -1; } else { @@ -105,8 +103,8 @@ cleanup: /* hack to let an user the time to cancel a download */ sleep(2); } - FREE(destfile); - FREE(tempfile); + free(destfile); + free(tempfile);
return(ret); }
On Sat, Apr 4, 2009 at 3:30 AM, Sebastian Nowickisebnow@gmail.com wrote:
I fixed the warnings and errors and tested it a bit more. Everything seems to be working fine. Using dynamic allocation for the xfercommand string removed the problem with PATH_MAX. The previously inlined functions are now separate functions as before, with minor changes due to the lack of macros from libalpm (see diff below).
So I finally got around to this. I made a few small changes, detailed in the diff below, that I included in the final patch.
diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 31e1f45..ce8c691 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -151,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); -
You simply forgot to kill these here.
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 9aa6141..6b163ce 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -267,12 +267,17 @@ static int download(const char *url, const char *localpath, time_t mtimeold, time_t *mtimenew) { if(handle->fetchcb == NULL) { #if defined(INTERNAL_DOWNLOAD) - handle->fetchcb = download_internal; + return(download_internal(url, localpath, mtimeold, mtimenew)); #else RET_ERR(PM_ERR_EXTERNAL_DOWNLOAD, -1); #endif + } else { + int ret = handle->fetchcb(url, localpath, mtimeold, mtimenew); + if(ret == -1) { + RET_ERR(PM_ERR_EXTERNAL_DOWNLOAD, -1); + } + return(ret); } - return handle->fetchcb(url, localpath, mtimeold, mtimenew); }
Structuring the code this way ensures the correct pm_errno value will propagate up the call stack.
/* diff --git a/src/pacman/conf.h b/src/pacman/conf.h index 2bf952a..2d3de98 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -73,7 +73,7 @@ typedef struct __config_t { unsigned short cleanmethod; /* select -Sc behavior */ alpm_list_t *holdpkg; alpm_list_t *syncfirst; - char *xfercommand; /* external download command */ + char *xfercommand; } config_t;
/* Operations */ diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index 7cf1396..7cecd90 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -657,7 +657,7 @@ int download_with_xfercommand(const char *url, const char *localpath, /* cwd to the download directory */ getcwd(cwd, PATH_MAX); if(chdir(localpath)) { - pm_printf(PM_LOG_DEBUG, "could not chdir to %s\n", localpath); + pm_printf(PM_LOG_WARNING, "could not chdir to %s\n", localpath); ret = -1; goto cleanup; } @@ -666,7 +666,7 @@ int download_with_xfercommand(const char *url, const char *localpath, retval = system(parsedCmd);
if(retval == -1) { - pm_printf(PM_LOG_DEBUG, "running XferCommand: fork failed!\n"); + pm_printf(PM_LOG_WARNING, "running XferCommand: fork failed!\n");
These two were warnings before; they are kind of a big deal.
ret = -1; } else if(retval != 0) { /* download failed */
-Dan
pacman-dev@lists.archlinux.org