From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (wildebeest.demon.nl [212.238.236.112]) by sourceware.org (Postfix) with ESMTPS id 754A9398B877 for ; Thu, 8 Jul 2021 14:43:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 754A9398B877 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from tarox.wildebeest.org (83-87-18-245.cable.dynamic.v4.ziggo.nl [83.87.18.245]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 12DB5302FBA6; Thu, 8 Jul 2021 16:43:15 +0200 (CEST) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id 7CF0F4003072; Thu, 8 Jul 2021 16:43:15 +0200 (CEST) Message-ID: <84304ca22a64e31485dda14667077975cb639cc7.camel@klomp.org> Subject: Re: [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. From: Mark Wielaard To: Alice Zhang , elfutils-devel@sourceware.org Date: Thu, 08 Jul 2021 16:43:15 +0200 In-Reply-To: <20210706201502.336113-1-alizhang@redhat.com> References: <20210706201502.336113-1-alizhang@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.28.5 (3.28.5-10.el7) Mime-Version: 1.0 X-Spam-Status: No, score=-8.3 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_BADIPHTTP, KAM_DMARC_STATUS, NORMAL_HTTP_TO_IP, NUMERIC_HTTP_ADDR, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Jul 2021 14:43:19 -0000 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). >=20 > Correponding test has been added to tests/run-debuginfod-find.sh >=20 > Signed-off-by: Alice Zhang 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 =20 Signed-off-by: Alice Zhang --- That way the first sentence (note the extra blank line) becomes the short commit message/subject. + /* If the verified_handle is NULL and rc !=3D -ENOENT, the query > fails with > + * an error code other than 404, then do several retry within the retr= y_limit. > + * Clean up all old handles and jump back to the beginning of query_in= _parallel, > + * reinitialize handles and query again.*/ > if (verified_handle =3D=3D NULL) > - goto out1; > + { > + if (rc !=3D -ENOENT && retry_limit-- > 0) > + { > + if (vfd >=3D 0) > + dprintf (vfd, "Retry failed query, %d attempt(s) remaining\n= ", retry_limit); > + /* remove all handles from multi */ > + for (int i =3D 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: =3D=3D24684=3D=3D 43,840 bytes in 10 blocks are definitely lost in loss rec= ord 61 of 61 =3D=3D24684=3D=3D at 0x4C29F73: malloc (vg_replace_malloc.c:309) =3D=3D24684=3D=3D by 0x52EB2A7: debuginfod_query_server (debuginfod-clie= nt.c:789) =3D=3D24684=3D=3D 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" > =20 > /* 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=3D`(testrun env DEBUGINFOD_URLS=3D > http://255.255.255.255/JUNKJUNK DEBUGINFOD_RETRY_LIMIT=3D10 > DEBUGINFOD_VERBOSE=3D1 \ > + ${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