Hi - > > I think it has a chance to be useful to other clients too, for example > > for other proxy / authentication schemes. And given that there is a > > shared library boundary, private APIs aren't in easy reach. > > "separately" as in separate commits? ... I suppose, if it really > > matters. > > Yes, please as a separate commit. It makes it so much easier on the > reviewers if the patches are smaller.
OK, trading merge pains I guess. > The reason I am slightly hesitant is because this feels like a feature > with corner cases that might not be clear. What about other headers > than Agent string if they have been set/will be set for example. The agent one is the only one that the client library currently sets on its own initiative by default. That's why it's a special case. The underlying libcurl api does not let one -remove- a custom header, only add new ones. > Could you expand a little on the use case? I see you set an X- > Forwarded-For header, but that seems it can be easily forged. I see > it might be interesting for testing, but would you use it in > production? These XFF headers are interesting when one wants to backtrace a federated request. Within a trusted tree of servers, it lets an administrator figure out the ultimate client, for blacklisting or special processing if necessary. So yes, absolutely in production. (It doesn't matter if the initial parts are forged by a client as long as the trailing parts are appended by the trusted tree.) > > Clarified this in a followup patch. No, only valid during the progress > > callback function itself. > > That is a pity, it seems useful without having to add a progress > function. Then we should probably make sure it doesn't return a value > in that case, because I suspect people will use it otherwise and will > complain if we break it later. It returns NULL. > But if we can make it work when the target_handle is valid, that would > be nice. The duration of a debuginfod_find_* call is exactly when the target_handle is valid. The latter gets cleaned up when the former finishes up. > > > > +/* Set the user parameter. */ > > > > +void debuginfod_set_user_data (debuginfod_client *client, void *value); > > > > + > > > > +/* Get the user parameter. */ > > > > +void* debuginfod_get_user_data (debuginfod_client *client); > > > > > > In theory I like these additions. But I don't really see the point of > > > how they are used. Is the only use case to pass the string "Progress"? > > > > That is for test coverage. > > Better to have a "real" test imho. Or at least comment it, so someone > doesn't clean up the code. It's a real test in that the testsuite asserts that the value is preserved. I can certainly clarify the comment. > But can we add this as a separate feature in a separate commit? OK. - FChE
