[pacman-dev] [PATCH v3] Parametrise the different ways in which the payload is reset

[Martin Kühne]

In FS#43434, Downloads which fail and are restarted on a different server
will resume and may display a negative download speed. The payload's progress
in libalpm was not properly reset which ultimately caused terminal noise
because the line width calculation assumes positive download speeds.

This patch fixes the incomplete reset of the payload by mimicing what
be_sync.c:alpm_db_update() does over in sync.c:download_single_file().
The new dload.c:_alpm_dload_payload_reset_for_retry() extends beyond the
current behavior by updating initial_size and prevprogress for this case.
This makes pacman reset the progress properly in the next invocation of the
callback and display positive download speeds.

Fixes FS#43434.

Signed-off-by: Martin Kühne 
---
 lib/libalpm/dload.c | 10 ++
 lib/libalpm/dload.h |  1 +
 lib/libalpm/sync.c  |  4 +---
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index dc57c92..855099b 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -762,4 +762,14 @@ void _alpm_dload_payload_reset(struct dload_payload 
*payload)
memset(payload, '\0', sizeof(*payload));
 }
 
+void _alpm_dload_payload_reset_for_retry(struct dload_payload *payload)
+{
+   ASSERT(payload, return);
+
+   FREE(payload->fileurl);
+   payload->initial_size += payload->prevprogress;
+   payload->prevprogress = 0;
+   payload->unlink_on_fail = 0;
+}
+
 /* vim: set noet: */
diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
index 427c486..3459665 100644
--- a/lib/libalpm/dload.h
+++ b/lib/libalpm/dload.h
@@ -47,6 +47,7 @@ struct dload_payload {
 };
 
 void _alpm_dload_payload_reset(struct dload_payload *payload);
+void _alpm_dload_payload_reset_for_retry(struct dload_payload *payload);
 
 int _alpm_download(struct dload_payload *payload, const char *localpath,
char **final_file, const char **final_url);
diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
index 00b68d0..a1f673f 100644
--- a/lib/libalpm/sync.c
+++ b/lib/libalpm/sync.c
@@ -946,9 +946,7 @@ static int download_single_file(alpm_handle_t *handle, 
struct dload_payload *pay
EVENT(handle, );
return 0;
}
-
-   FREE(payload->fileurl);
-   payload->unlink_on_fail = 0;
+   _alpm_dload_payload_reset_for_retry(payload);
}
 
event.type = ALPM_EVENT_PKGDOWNLOAD_FAILED;
-- 
2.10.0

Re: [pacman-dev] [PATCH v3 3/4] Declare alpm_pkg_get_(origin|reason) to return int

[Andrew Gregory]

On 10/17/16 at 10:17pm, Allan McRae wrote:
> On 13/10/16 06:13, ivy.fos...@gmail.com wrote:
> > From: Ivy Foster 
> > 
> > These functions can return -1 on error, which is not included in the
> > enumerated types they were declared to return.
> > 
> > Signed-off-by: Ivy Foster 
> > ---
> 
> 
> be_local.c: In function ‘alpm_pkg_set_reason’:
> be_local.c:1124:30: error: comparison between signed and unsigned
> integer expressions [-Werror=sign-compare]
>   if(alpm_pkg_get_reason(pkg) == reason) {
>   ^~
> 
> You can use --enable-warningflags to build with many of our current
> warning flags enabled.
> 
> Why change the return to an int and not add an UNKNOWN value to the enum
> (with value 0 or 1 << 30)?
> 
> Allan

The only way get_reason and get_origin return -1 is if pkg is NULL, so
UNKNOWN isn't really accurate.  We could add an ERROR value, but I was
hoping to avoid that.

apg

Re: [pacman-dev] [PATCH v2] Parametrise the different ways in which the payload is reset

[Dave Reisner]

On Fri, Oct 14, 2016 at 09:26:46PM +0200, mar77i wrote:
> In FS#43434, Downloads which fail and are restarted on a different server
> will resume and may display a negative download speed. The payload's progress
> in libalpm was not properly reset which ultimately caused terminal noise
> because the line width calculation assumes positive download speeds.
> 
> This patch fixes the incomplete reset of the payload by mimicing what
> be_sync.c:alpm_db_update() does over in sync.c:download_single_file().
> dload.c:_alpm_dload_payload_reset() bundles and extends over the current
> behavior by updating initial_size and prevprogress for this case.
> This makes pacman reset the progress properly in the next invocation of the
> callback and display positive download speeds.
> 
> Fixes FS#43434.
> 
> Signed-off-by: Martin Kühne 
> ---
>  lib/libalpm/be_sync.c |  4 ++--
>  lib/libalpm/dload.c   | 15 +++
>  lib/libalpm/dload.h   |  2 +-
>  lib/libalpm/sync.c|  4 +---
>  4 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
> index a836277..2425359 100644
> --- a/lib/libalpm/be_sync.c
> +++ b/lib/libalpm/be_sync.c
> @@ -244,7 +244,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
>   payload.unlink_on_fail = 1;
>  
>   ret = _alpm_download(, syncpath, NULL, _db_url);
> - _alpm_dload_payload_reset();
> + _alpm_dload_payload_reset(, 0);
>   updated = (updated || ret == 0);
>  
>   if(ret != -1 && updated && (level & ALPM_SIG_DATABASE)) {
> @@ -300,7 +300,7 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
>   sig_ret = _alpm_download(, syncpath, NULL, 
> NULL);
>   /* errors_ok suppresses error messages, but not the 
> return code */
>   sig_ret = payload.errors_ok ? 0 : sig_ret;
> - _alpm_dload_payload_reset();
> + _alpm_dload_payload_reset(, 0);

This is kind of opaque at a glance. What does 0 mean? Potential
modifications:

1) Add an inline comment:

  _alpm_dload_payload_reset(, 0 /* do not allow resume */);

2) Separate this out into 2 wrappers:

  _alpm_dload_payload_reset_for_retry();
  _alpm_dload_payload_reset();
  _alpm_dload_payload_reset_internal(, 0|1);

I don't feel too strongly about this, though, since this sort of
opaqueness is evident elsewhere in the codebase.

>   }
>  
>   if(ret != -1 && sig_ret != -1) {
> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index dc57c92..55ff41f 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -697,7 +697,7 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, 
> const char *url)
>  
>   /* download the file */
>   ret = _alpm_download(, cachedir, _file, 
> _pkg_url);
> - _alpm_dload_payload_reset();
> + _alpm_dload_payload_reset(, 0);
>   if(ret == -1) {
>   _alpm_log(handle, ALPM_LOG_WARNING, _("failed to 
> download %s\n"), url);
>   free(final_file);
> @@ -738,7 +738,7 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, 
> const char *url)
>   FREE(sig_final_file);
>   }
>   free(sig_filepath);
> - _alpm_dload_payload_reset();
> + _alpm_dload_payload_reset(, 0);
>   }
>  
>   /* we should be able to find the file the second time around */
> @@ -750,15 +750,22 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t 
> *handle, const char *url)
>   return filepath;
>  }
>  
> -void _alpm_dload_payload_reset(struct dload_payload *payload)
> +void _alpm_dload_payload_reset(struct dload_payload *payload, int 
> allow_resume)
>  {
>   ASSERT(payload, return);
>  
> + FREE(payload->fileurl);
> + if(allow_resume) {
> + payload->initial_size += payload->prevprogress;
> + payload->prevprogress = 0;
> + payload->unlink_on_fail = 0;
> + return;
> + }
> +
>   FREE(payload->remote_name);
>   FREE(payload->tempfile_name);
>   FREE(payload->destfile_name);
>   FREE(payload->content_disp_name);
> - FREE(payload->fileurl);
>   memset(payload, '\0', sizeof(*payload));
>  }
>  
> diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
> index 427c486..c9c94b8 100644
> --- a/lib/libalpm/dload.h
> +++ b/lib/libalpm/dload.h
> @@ -46,7 +46,7 @@ struct dload_payload {
>  #endif
>  };
>  
> -void _alpm_dload_payload_reset(struct dload_payload *payload);
> +void _alpm_dload_payload_reset(struct dload_payload *payload, int 
> allow_resume);
>  
>  int _alpm_download(struct dload_payload *payload, const char *localpath,
>   char **final_file, const char **final_url);
> diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
> index 00b68d0..81900df 100644
> --- 

Re: [pacman-dev] [PATCH v3 2/4] Represent bitfields as ints, not enums

[Allan McRae]

On 13/10/16 06:13, ivy.fos...@gmail.com wrote:
> From: Ivy Foster 
> 
> Many bitfield variables are declared to be enums, because they are
> generated using bitwise operations on enums such. However, their
> actual values aren't necessary members of their parent enum, so
> declaring them 'int' is more accurate.
>

Looks good to me.

A

Re: [pacman-dev] [PATCH] doc/pacman.8.txt: improve description of -Qt

[Allan McRae]

On 15/10/16 08:13, ivy.fos...@gmail.com wrote:
> From: Ivy Foster 
> 
> Though correct, the wording of the description of Query's
> -t/--unrequired option was confusing. Closes FS#48144.
> 
> Signed-off-by: Ivy Foster 
> ---

Thanks.

A

Re: [pacman-dev] [PATCH v3] makepkg: print files with refs to $srcdir/$pkgdir

[Allan McRae]

On 16/10/16 09:34, ivy.fos...@gmail.com wrote:
> From: Ivy Foster 
> 
> Since rewriting build_references() anyway, tweaked quoting.
> Implements FS#31558.
> 
> Signed-off-by: Ivy Foster 
> ---
>  scripts/libmakepkg/lint_package/build_references.sh.in | 16 +---
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/scripts/libmakepkg/lint_package/build_references.sh.in 
> b/scripts/libmakepkg/lint_package/build_references.sh.in
> index 67c14e6..b9958c8 100644
> --- a/scripts/libmakepkg/lint_package/build_references.sh.in
> +++ b/scripts/libmakepkg/lint_package/build_references.sh.in
> @@ -25,14 +25,16 @@ LIBRARY=${LIBRARY:-'@libmakepkgdir@'}
>  
>  source "$LIBRARY/util/message.sh"
>  
> -
>  lint_package_functions+=('warn_build_references')
>  
>  warn_build_references() {
> - if find "${pkgdir}" -type f -print0 | xargs -0 grep -q -I "${srcdir}" ; 
> then
> - warning "$(gettext "Package contains reference to %s")" 
> "\$srcdir"
> - fi
> - if find "${pkgdir}" -type f -print0 | xargs -0 grep -q -I 
> "${pkgdirbase}" ; then
> - warning "$(gettext "Package contains reference to %s")" 
> "\$pkgdir"
> - fi
> + local refs
> +
> + for var in srcdir pkgdir; do
> + mapfile -t refs < <(find "$pkgdir" -type f -exec grep -l 
> "${!var}" {} +)
> + if  [[ ${#refs} -gt 0 ]]; then

Our style used (( )) style tests for number tests.  So this will become

if (( ${#refs} > 0 )); then

I will make this change when I apply.

Thanks,
Allan