Hi Alice,
On Mon, 2020-06-29 at 09:47 -0400, Alice Zhang via Elfutils-devel
wrote:
> Signed-off-by: Alice Zhang <alizh...@redhat.com>
> ---
> debuginfod/debuginfod-client.c | 17 ++++++++---------
> tests/run-debuginfod-find.sh | 6 ++++++
> 2 files changed, 14 insertions(+), 9 deletions(-)
The commit message is nice, but should not be one single line.
Start with a one sentence description (prefixed with the component
we are modifying libelf, libdw, debuginfod, etc.). Then a blank line
and then one (or more) paragraphs, describing the intent of the patch.
Should be short sentences (<72 characters) ideally.
Specifics of what precisely changed per file can go into the ChangeLog
file.
I would probably write it as follows:
debuginfod: DEBUGINFOD_URLS should accept scheme-free urls
Check scheme instead of effective url so that user may abbreviate
DEBUGINFOD_URL. Add one test for scheme free http url.
Notice that libcurl does not provide an almighty scheme free url
support, /path/to/something without FILE:// can not be recognized
in most circumstances, therefore for the neatness of our code
structure, DEBUGINFOD_ URL of scheme "FILE" must be input as URI.
Signed-off-by: Alice Zhang <alizh...@redhat.com>
One question about that sentence:
> Notice that libcurl does not provide an almighty scheme free url
> support, /path/to/something without FILE:// can not be recognized
> in most circumstances, therefore for the neatness of our code
> strucuture, DEBUGINFOD_ URL of scheme "FILE" must be input as URI.
Maybe you could add an example of what scheme free URLs work and which
don't?
> diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-
> client.c
> index c2aa4e10..3764b5f2 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -722,7 +722,6 @@ debuginfod_query_server (debuginfod_client *c,
> else
> snprintf(data[i].url, PATH_MAX, "%s%s/%s/%s", server_url,
> slashbuildid, build_id_bytes, type);
> -
Please avoid whitespace changes like this or changing tabs to spaces.
Whitespace isn't always consistently used. But please keep it as it is
to help with seeing the actual code changes. It also helps when using
git annotate on a file.
> curl_easy_setopt(data[i].handle, CURLOPT_URL, data[i].url);
> curl_easy_setopt(data[i].handle,
> CURLOPT_WRITEFUNCTION,
> @@ -867,29 +866,29 @@ debuginfod_query_server (debuginfod_client *c,
>
> if (msg->easy_handle != NULL)
> {
> - char *effective_url = NULL;
> long resp_code = 500;
> + char *scheme = NULL;
> CURLcode ok1 = curl_easy_getinfo (target_handle,
> - CURLINFO_EFFECTIVE_URL,
> - &effective_url);
> - CURLcode ok2 = curl_easy_getinfo (target_handle,
> CURLINFO_RESPONSE_CODE,
> &resp_code);
> - if(ok1 == CURLE_OK && ok2 == CURLE_OK && effective_url)
> + CURLcode ok2 = curl_easy_getinfo (target_handle,
> + CURLINFO_SCHEME,
> + &scheme);
Unfortunately CURLINFO_SCHEME is only available since libcurl 7.52.0.
So this won't compile on older systems (like RHEL7, which only has
7.29).
This is a bit of a bummer. Since it seems CURLINFO_EFFECTIVE_URL
doesn't always include the scheme because of this bug in newer libcurl:
https://github.com/curl/curl/issues/4491
So we cannot use CURLINFO_EFFECTIVE_URL to get the scheme because it is
broken on some systems, and we cannot use CURLINFO_SCHEME because it
doesn't exist on some systems :{
Maybe we can try CURLINFO_EFFECTIVE_URL first and if that fails and
CURLINFO_SCHEME is defined.
Note that older CURLINFO_RESPONSE_CODE (libcurl-7.29.0) does add the
scheme when it was missing, but in UPPERCASE (so "example.com" becomes
"HTTP://example.com" and in newer (libcurl-7.69) does add it in
lowercase (so it becomes "http://example.com";).
So maybe we should just use CURLINFO_RESPONSE_CODE and strncasecmp?
And simply ignore the broken libcurl versions?
> + if(ok1 == CURLE_OK && ok2 == CURLE_OK && scheme)
> {
> - if (strncmp (effective_url, "http", 4) == 0)
> + if (strncmp (scheme, "HTTP", 4) == 0)
> if (resp_code == 200)
> {
> verified_handle = msg->easy_handle;
> break;
> }
> - if (strncmp (effective_url, "file", 4) == 0)
> + if (strncmp (scheme, "FILE", 4) == 0)
Note the whitespace changes, just keep using all space. Even though I
agree using tabs is nicer :)
So I think using effective_url with strncasecmp instead of strncmp
might work here with most libcurl versions. If you want to be more
prudent then you can also check for the scheme when that fails with
#ifdef CURLINFO_SCHEME
> if (resp_code == 0)
> {
> verified_handle = msg->easy_handle;
> break;
> }
> - }
> + }
Another whitespace only change.
> }
> }
> }
Thanks,
Mark
