[PATCH] 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.

[Mark Wielaard]

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 <alizhang@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 <alizhang@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