Hi -
> I see how this is useful for logging what is going on in the debuginfod
> server. And even how some client program might want to set the User-
> Agent itself. But it feels a bit like a hack to make this a public API
> for the client library.
Sure.
> Normally a user client doesn't really need to know much about whether
> or how the file information is fetched. But in this case we have a http
> transport specific thing (and we just added support for file:// URLs).
> It also seems very curl specific. To use it correctly you need to know
> the structure of HTTP request header/value and how CURLOPT_HTTPHEADER
> uses the given string to add, replace, delete (end with a colon) or
> create a header with no value (adding a semicolon at the end).
I don't see why this would be true. The function is named "add", and
the documentation says "Header: value". This does not expose CURL
replace/delete operations.
> I think that we should be very explicit that this API is an optional
> hint only. That the user must not rely on the header actually being
> used or forwarded through the transport. And that it is only to be used
> for optional logging.
I guess I don't see a problem here. HTTP request headers are by
definition optional things. How it's used --- why would we want to
dictate "only ... for" anything? We do what we do.
> I am especially worried that people will start using the API to
> override and/or disable internal (curl) headers, which would mean we
> are stuck with all kinds of curl specifics. So ideally I would like to
> programmatically prevent that. Or at document very clearly that if
> someone does that, that is totally unsupported and we are free to break
> such usage in any (minor) update/bug fix release.
People who manage to muck up curl internal headers would already be
violating our documentation ("add", "Header: value"). I just don't
see any threat here, let alone any sort of special technical
obligation on us to stop this or offer scare stories. The function
simply does what it says on the tin.
> > void
> > debuginfod_end (debuginfod_client *client)
> > {
> > + if (client) /* make it safe to be invoked with NULL */
> > + curl_slist_free_all (client->headers);
> > free (client->url);
> > free (client);
> > }
>
> It seems a good idea to allow debuginfod_end (NULL). But if client
> would be NULL then the free (client->url) would also crash.
> So just put everything under the if (client) { ... }
Or if (!client) return or whatever.
> > {
> > + add_default_headers(client);
> > return debuginfod_query_server(client, build_id, build_id_len,
> > "source", filename, path);
> > }
>
> Why not have add_default_headers at the top of the
> debuginfod_query_server () function instead of repeating it 3 times
> here?
Sure, OK.
> So if there is any way to check whether this is overriding and/or
> deleting an existing internal curl header here it would be really good
> to add that here and simply reject that.
I guess I don't see why we would voluntarily undertake the
responsibility for detailed classification of already
documentation-violating input. Remember, this is just a client. If
some user really needs this sort of goofy capability, they could have
always just written their code to libcurl directly. We can't stop
them.
> > struct MHD_Response *r = 0;
> > try
> > {
> > - r = handle_buildid (buildid, "debuginfo", "", &alt_fd);
> > + r = handle_buildid (0, buildid, "debuginfo", "", &alt_fd);
> > }
>
> 0 instead of NULL?
> I believe we went over this before, 0 is more C++?
Yes.
> > +/* Add an outgoing HTTP request "Header: Value". Copies string. */
> > +int debuginfod_add_http_header (debuginfod_client *client, const char*
> > header);
>
> So here at least add "optional header" or something. A warning that
> this is a hint only and might be ignored depending on backend/transport
> used.
Adding a http header to a non-http transport URL is obviously
meaningless and harmless. I don't see why we should belabour it or
give people any concern about it.
- FChE
