Hi Alice,
On Tue, 2021-07-06 at 16:15 -0400, Alice Zhang via Elfutils-devel
wrote:
> Also introduced DEBUGINFOD_RETRY_LIMIT_ENV_VAR and default
> DEBUGINFOD_RETRY_LIMIT(which is 2).
>
> Correponding test has been added to tests/run-debuginfod-find.sh
>
> Signed-off-by: Alice Zhang <alizh...@redhat.com>
Nice. But try to split up the first paragraph. e.g.
---
PR27531: retry within default retry_limit will be supported.
In debuginfod-client.c (debuginfod_query_server), insert a
goto statement for jumping back to the beginning of curl
handles set up if query fails and a non ENOENT error is
returned.
Also introduced DEBUGINFOD_RETRY_LIMIT_ENV_VAR and default
DEBUGINFOD_RETRY_LIMIT (which is 2).
Correponding test has been added to tests/run-debuginfod-find.sh
Signed-off-by: Alice Zhang <alizh...@redhat.com>
---
That way the first sentence (note the extra blank line) becomes the
short commit message/subject.
+ /* If the verified_handle is NULL and rc != -ENOENT, the query
> fails with
> + * an error code other than 404, then do several retry within the
> retry_limit.
> + * Clean up all old handles and jump back to the beginning of
> query_in_parallel,
> + * reinitialize handles and query again.*/
> if (verified_handle == NULL)
> - goto out1;
> + {
> + if (rc != -ENOENT && retry_limit-- > 0)
> + {
> + if (vfd >= 0)
> + dprintf (vfd, "Retry failed query, %d attempt(s) remaining\n",
> retry_limit);
> + /* remove all handles from multi */
> + for (int i = 0; i < num_urls; i++)
> + {
> + curl_multi_remove_handle(curlm, data[i].handle); /* ok to
> repeat */
> + curl_easy_cleanup (data[i].handle);
> + }
> + goto query_in_parallel;
> + }
> + else
> + goto out1;
> + }
You also need to call free (data) before the goto query_in_parallel
since that will also be reallocated. Or you can rearrange the code a
little around query_in_parallel to keep the data around, but only
reinitialize it, but I think it is fine to free and then malloc again.
Try to run under valgrind --full-leak-check and you'll see:
==24684== 43,840 bytes in 10 blocks are definitely lost in loss record 61 of 61
==24684== at 0x4C29F73: malloc (vg_replace_malloc.c:309)
==24684== by 0x52EB2A7: debuginfod_query_server (debuginfod-client.c:789)
==24684== by 0x401131: main (debuginfod-find.c:192)
(P.S. Don't worry about the still reachable issues.)
diff --git a/debuginfod/debuginfod.h.in b/debuginfod/debuginfod.h.in
> index 559ea947..282e523d 100644
> --- a/debuginfod/debuginfod.h.in
> +++ b/debuginfod/debuginfod.h.in
> @@ -35,6 +35,7 @@
> #define DEBUGINFOD_TIMEOUT_ENV_VAR "DEBUGINFOD_TIMEOUT"
> #define DEBUGINFOD_PROGRESS_ENV_VAR "DEBUGINFOD_PROGRESS"
> #define DEBUGINFOD_VERBOSE_ENV_VAR "DEBUGINFOD_VERBOSE"
> +#define DEBUGINFOD_RETRY_LIMIT_ENV_VAR "DEBUGINFOD_RETRY_LIMIT"
>
> /* The libdebuginfod soname. */
> #define DEBUGINFOD_SONAME "@LIBDEBUGINFOD_SONAME@"
Could you also add an entry for DEBUGINFOD_RETRY_LIMIT to
doc/debuginfod_find_debuginfo.3 under ENVIRONMENT VARIABLES.
> +####################################################################
> ####
> +# set up tests for retrying failed queries.
> +retry_attempts=`(testrun env DEBUGINFOD_URLS=
> http://255.255.255.255/JUNKJUNK DEBUGINFOD_RETRY_LIMIT=10
> DEBUGINFOD_VERBOSE=1 \
> + ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo
> /bin/ls || true) 2>&1 >/dev/null \
> + | grep -c 'Retry failed query'`
> +if [ $retry_attempts -ne 10 ]; then
> + echo "retry mechanism failed."
> + exit 1;
> + fi
> +
> exit 0
Cute testcase. This works because 255.255.255.255 will always fail.
Thanks,
Mark
