Re: PR25369 rfc: debuginfod client api extension for progressfn prettying etc.

[Mark Wielaard <mark@klomp.org>]

Hi Frank,

On Tue, 2020-02-25 at 10:32 -0500, Frank Ch. Eigler wrote:
> > > +/* Add an outgoing HTTP request  "Header: Value".  Copies string.  */
> > > +int debuginfod_add_http_header (debuginfod_client *client, const char* header);
> > 
> > This one seems different from the others and has a specific use case
> > just for the debuginfod server. Are you sure it is generic enough to be
> > added as a new public interface? If we add this can we do it separately
> > from other debuginfo-client progress improvements?
> 
> 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.

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.

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?

> > > +/* Return currently active URL, if known.  String owned by curl, do not free.  */
> > > +const char* debuginfod_get_url (debuginfod_client *client);
> > 
> > This does seem useful with the comment that was already made, that
> > lifetime of the returned string should be documented. I assume it is
> > valid to call this after debuginfod_find_* has returned, but before
> > debuginfod_end has been called?
> 
> 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.

But if we can make it work when the target_handle is valid, that would
be nice.

> > > +/* 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.

> > If there are no real users for this then I think we should not add
> > these at this time. Or is some other client using them? I am not
> > really
> > against it, but would prefer if we add them separately to keep the
> > patches concise.
> 
> GDB would use them pretty much immediately, to be able to prepare a
> more informative progress notification (like the file name whose
> debuginfo is being sought, instead of its buildid).

OK. Yes, that seems like a good use case.
But can we add this as a separate feature in a separate commit?

Thanks,

Mark