Hi Aaron, On Tue, Nov 01, 2022 at 12:53:41AM -0400, Aaron Merey wrote: > Thanks again for the detailed review. I fixed the issues you pointed out.
This version looks really good. Please push, so it is included in the release tomorrow. > On Sat, Oct 29, 2022 at 8:29 PM Mark Wielaard <m...@klomp.org> wrote: > > > @@ -1008,6 +1206,9 @@ debuginfod_query_server (debuginfod_client *c, > > > snprintf(data[i].url, PATH_MAX, "%s/%s/%s/%s", server_url, > > > build_id_bytes, type, escaped_string); > > > } > > > + else if (section) > > > + snprintf(data[i].url, PATH_MAX, "%s/%s/%s/%s", server_url, > > > + build_id_bytes, type, section); > > > > Does the section part need to be path escaped? Like the filename is > > with curl_easy_escape? And if it is how does that interact with the > > cache file name? > > I assumed that section names would not contain '/'. However I noticed > that if we call debuginfod_find_section with a section name containing > '/', it will return -EINVAL. This triggers the downloading of debuginfo > in order to attempt client-side extraction. This is a waste so I changed > the client to path-escape the section name for caching purposes just like > the source query filename. It is highly unlikely a '/' is in the section name, but good to have this covered anyway. Nicely extracted that escape function. > > > + /* The servers may have lacked support for section queries. Attempt to > > > + download the debuginfo or executable containing the section in order > > > + to extract it. */ > > > > This does somewhat defeat the purpose of just getting the section data > > of course. So we could also just return EINVAL to the user here, so > > they have to get the whole debuginfo themselves. But maybe just > > pretending it worked is fine to keep the code path of the user > > simpler? > > This keeps the code path simpler for the user and like Frank mentioned > it enables an intermediate server to extract the section and provide > it to the client. This server will also cache the debuginfo/executable, > possibly speeding up future requests. Ah, yes, the debuginfod server doesn use the client library too. Thanks, Mark
