Re: [pacman-dev] [PATCH] Avoid depending on side effects in assert(...) expressions
On 14/5/20 4:43 am, Dave Reisner wrote:
> When building with -DNDEBUG, assert statements are compiled out to
> no-ops. Thus, we can't depend on assignments or other computations
> occurring inside the assert().
> ---
Thanks.
> It's perhaps worth mentioning that nowhere else in the ALPM codebase
> do we use assert().
Yes - but we really don't have a way to pass back from our callback to
libalpm. So I am happy with the "shit hit the fan" approach until that
changes.
> src/pacman/callback.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/pacman/callback.c b/src/pacman/callback.c
> index 25909e02..4240a779 100644
> --- a/src/pacman/callback.c
> +++ b/src/pacman/callback.c
> @@ -862,12 +862,14 @@ static void dload_progress_event(const char *filename,
> alpm_download_event_progr
> int64_t curr_time = get_time_ms();
> double last_chunk_rate;
> int64_t timediff;
> + bool ok;
>
> if(!dload_progressbar_enabled()) {
> return;
> }
>
> - assert(find_bar_for_filename(filename, , ));
> + ok = find_bar_for_filename(filename, , );
> + assert(ok);
>
> /* compute current average values */
> timediff = curr_time - bar->sync_time;
> @@ -902,12 +904,14 @@ static void dload_complete_event(const char *filename,
> alpm_download_event_compl
> int index;
> struct pacman_progress_bar *bar;
> int64_t timediff;
> + bool ok;
>
> if(!dload_progressbar_enabled()) {
> return;
> }
>
> - assert(find_bar_for_filename(filename, , ));
> + ok = find_bar_for_filename(filename, , );
> + assert(ok);
> bar->completed = true;
>
> /* This may not have been initialized if the download finished before
>
Re: [pacman-dev] [PATCH] Avoid depending on side effects in assert(...) expressions
On 14/5/20 12:54 pm, Andrew Gregory wrote: > The > callback api should be fixed so that callbacks can indicate an error > that should cancel the download. Probably - there are a lot of points in the codebase where we pass an error back but don't really do much with it... Or don't pass back an error. > I'd also say we should be passing the full list of download items to > the callback so that it's possible to write at least a very simple > multi-download callback without having to maintain a bunch of > complicated state information. I was fairly happy with the multi-download callback. It was at a point that it worked and was functionally correct (as far as I could tell), but there are likely ways to make it better such as your idea. However, I'm not letting that stop anything good and working being committed to the tree as improvements can happen later. Perfect being the enemy of good and all that. A
Re: [pacman-dev] [PATCH] Avoid depending on side effects in assert(...) expressions
On 05/13/20 at 12:08pm, Anatol Pomozov wrote: > > --- > > It's perhaps worth mentioning that nowhere else in the ALPM codebase > > do we use assert(). > > I quite like the idea of defensive programming. This is something that > I learnt the hard way when I was working with chips firmware. > So I often add additional checks across the codebase and it saves me > time during active phase of development/refactoring. ... > A bit of context for this assert(). Both "progress" and "complete" > events should always have a corresponding "init" event where > progressbar structure is initialized. > > If callback.c received a "progress" event for a non-existent > progressbar then it is a programming error. The problem is not defensive programming, it's that assert gets compiled out under -DNDEBUG, so all your defenses disappear. You say that the only way there would not be a corresponding progress bar is if the progress event is called without an init event; that's not true. If there's a memory allocation failure in the init event there won't be a progress object. At that point bar and index are indeterminate and the best we can hope for is a quick segfault. The callback api should be fixed so that callbacks can indicate an error that should cancel the download. I'd also say we should be passing the full list of download items to the callback so that it's possible to write at least a very simple multi-download callback without having to maintain a bunch of complicated state information.
[pacman-dev] [PATCH] Move signature payload creation to download engine
Until now callee of ALPM download functionality has been in charge of
payload creation both for the main file (e.g. *.pkg) and for the accompanied
*.sig file. One advantage of such solution is that all payloads are
independent and can be fetched in parallel thus exploiting the maximum
level of download parallelism.
To build *.sig file url we've been using a simple string concatenation:
$requested_url + ".sig". Unfortunately there are cases when it does not
work. For example an archlinux.org "Download From Mirror" link looks like
this https://www.archlinux.org/packages/core/x86_64/bash/download/ and
it gets redirected to some mirror. But if we append ".sig" to the end of
the link url and try to download it then archlinux.org returns 404 error.
To overcome this issue we need to follow redirects for the main payload
first, find the final url and only then append '.sig' suffix.
This implies 2 things:
- the signature payload initialization need to be moved to dload.c
as it is the place where we have access to the resolved url
- *.sig is downloaded serially with the main payload and this reduces
level of parallelism
Move *.sig payload creation to dload.c. Once the main payload is fetched
successfully we check if the callee asked to download the accompanied
signature. If yes - create a new payload and add it to mcurl.
*.sig payload does not use server list of the main payload and thus does
not support mirror failover. *.sig file comes from the same server as
the main payload.
Refactor event loop in curl_multi_download_internal() a bit. Instead of
relying on curl_multi_check_finished_download() to return number of new
payloads we simply rerun the loop iteration one more time to check if
there are any active downloads left.
---
lib/libalpm/be_sync.c | 34 +++-
lib/libalpm/dload.c | 91 ++-
lib/libalpm/dload.h | 4 +-
3 files changed, 65 insertions(+), 64 deletions(-)
diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
index 82018e15..41675d21 100644
--- a/lib/libalpm/be_sync.c
+++ b/lib/libalpm/be_sync.c
@@ -180,12 +180,10 @@ int SYMEXPORT alpm_db_update(alpm_handle_t *handle,
alpm_list_t *dbs, int force)
dbforce = 1;
}
- CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle,
ALPM_ERR_MEMORY, cleanup));
+ siglevel = alpm_db_get_siglevel(db);
- /* set hard upper limit of 128 MiB */
- payload->max_size = 128 * 1024 * 1024;
+ CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle,
ALPM_ERR_MEMORY, cleanup));
payload->servers = db->servers;
-
/* print server + filename into a buffer */
len = strlen(db->treename) + strlen(dbext) + 1;
MALLOC(payload->filepath, len,
@@ -194,31 +192,11 @@ int SYMEXPORT alpm_db_update(alpm_handle_t *handle,
alpm_list_t *dbs, int force)
payload->handle = handle;
payload->force = dbforce;
payload->unlink_on_fail = 1;
-
+ payload->download_signature = (siglevel & ALPM_SIG_DATABASE);
+ payload->signature_optional = (siglevel &
ALPM_SIG_DATABASE_OPTIONAL);
+ /* set hard upper limit of 128 MiB */
+ payload->max_size = 128 * 1024 * 1024;
payloads = alpm_list_add(payloads, payload);
-
- siglevel = alpm_db_get_siglevel(db);
- if(siglevel & ALPM_SIG_DATABASE) {
- struct dload_payload *sig_payload;
- CALLOC(sig_payload, 1, sizeof(*sig_payload),
GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
- sig_payload->signature = 1;
-
- /* print filename into a buffer (leave space for
separator and .sig) */
- len = strlen(db->treename) + strlen(dbext) + 5;
- MALLOC(sig_payload->filepath, len,
- FREE(sig_payload); GOTO_ERR(handle,
ALPM_ERR_MEMORY, cleanup));
- snprintf(sig_payload->filepath, len, "%s%s.sig",
db->treename, dbext);
-
- sig_payload->handle = handle;
- sig_payload->force = dbforce;
- sig_payload->errors_ok = (siglevel &
ALPM_SIG_DATABASE_OPTIONAL);
-
- /* set hard upper limit of 16 KiB */
- sig_payload->max_size = 16 * 1024;
- sig_payload->servers = db->servers;
-
- payloads = alpm_list_add(payloads, sig_payload);
- }
}
event.type = ALPM_EVENT_DB_RETRIEVE_START;
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index 4dbb011f..b68dcf6d 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -18,6 +18,7 @@
* along with this program. If not, see
[pacman-dev] [PATCH] Avoid depending on side effects in assert(...) expressions
When building with -DNDEBUG, assert statements are compiled out to
no-ops. Thus, we can't depend on assignments or other computations
occurring inside the assert().
---
It's perhaps worth mentioning that nowhere else in the ALPM codebase
do we use assert().
src/pacman/callback.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/pacman/callback.c b/src/pacman/callback.c
index 25909e02..4240a779 100644
--- a/src/pacman/callback.c
+++ b/src/pacman/callback.c
@@ -862,12 +862,14 @@ static void dload_progress_event(const char *filename,
alpm_download_event_progr
int64_t curr_time = get_time_ms();
double last_chunk_rate;
int64_t timediff;
+ bool ok;
if(!dload_progressbar_enabled()) {
return;
}
- assert(find_bar_for_filename(filename, , ));
+ ok = find_bar_for_filename(filename, , );
+ assert(ok);
/* compute current average values */
timediff = curr_time - bar->sync_time;
@@ -902,12 +904,14 @@ static void dload_complete_event(const char *filename,
alpm_download_event_compl
int index;
struct pacman_progress_bar *bar;
int64_t timediff;
+ bool ok;
if(!dload_progressbar_enabled()) {
return;
}
- assert(find_bar_for_filename(filename, , ));
+ ok = find_bar_for_filename(filename, , );
+ assert(ok);
bar->completed = true;
/* This may not have been initialized if the download finished before
--
2.26.2
