[PATCH] D111252: [llvm] [Support] [Debuginfo] Add http and debuginfod client libraries and llvm-debuginfod-find tool

[Noah Shutty via Phabricator via llvm-commits]

noajshu marked 2 inline comments as done.
noajshu added inline comments.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:50-52
+#define HEADER_PREFIX "Content-Length: "
+#define HEADER_PREFIX_LEN 16
+  if (StringRef(Contents, HEADER_PREFIX_LEN) == HEADER_PREFIX) {
----------------
noajshu wrote:
> phosek wrote:
> > I'd avoid the macros which we don't usually use in C++ and instead use the `StringRef` methods, so you can do something like:
> > 
> > ```
> > StringRef Header(Contents, NMemb);
> > if (Header.starts_with("Content-Length: ")) {
> >   StringRef Length = Header.substr(16);
> >   size_t ContentLength;
> >   if (!Length.getAsInteger(10, ContentLength)) {
> >     ...
> >   }
> > }
> > ```
> > 
> > We should also make the code more resilient to handle potential issues, for example could the header look like `Content-Length    :     N` or is it guaranteed to always be `Content-Length: N`?
> Thanks for pointing out the variations in header formatting. I took a look at the [[ https://www.ietf.org/rfc/rfc2616.txt | HTTP/1.1 specification ]] and changed the parser to use `startswith_insensitive` as header names are case insensitive as well.
> To handle extra whitespace perhaps we could use a case-insensitive regex match like `content-length\s*?:\s*?(\d+)`. This would also give us the digits of the length as a match group.
I have changed this to use a Regex instead of a `startswith` test.


================
Comment at: llvm/lib/Support/HTTPClient.cpp:52
+#define HEADER_PREFIX_LEN 16
+  if (StringRef(Contents, HEADER_PREFIX_LEN) == HEADER_PREFIX) {
+    size_t ContentLength = std::stoll(
----------------
phosek wrote:
> If the header doesn't contain `Content-Length`, would we fail in `writeMemoryCallback` because we haven't allocated the buffer? I think the should be more resilient to handle that situation somehow (even if just by returning an error).
Instead of asserting the buffer has been allocated, I changed it to terminate the download and return an Error. However I agree we ought to be more resilient in this case especially since Transfer-Encoding is a valid alternative to Content-Length.
Perhaps we could fall back to repeated re-allocations in the case there is no Content-Length header?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111252/new/

https://reviews.llvm.org/D111252