Hi Frank, On Mon, 2020-02-24 at 22:35 -0500, Frank Ch. Eigler wrote: > As a part of PR25369, I propose this small set of client api > extensions, requested by gdb developers and needed by personal > experience. (I plan to roll it out on my debuginfod servers shortly > to let it soak.) An end-user visible immediate difference is in the > DEBUGINFOD_PROGRESS=1 format message, which now looks like this: > > Downloading from debuginfod / > Downloading from debuginfod - > Downloading from debuginfod \ > Downloading from > http://very:8222/buildid/817dcbd2ce0ecce19f22cbe5e136b6d9196428aa/executable > 433130328/433130328 > > This latter is a bit long and should probably be abbreviated, wdyt?
Might want to abbrev the build-id part to /81..aa/. The interesting part is which server is used imho. > I'd start reviewing this from the debuginfod.h header file outward. OK. Lets start there: > diff --git a/debuginfod/debuginfod.h b/debuginfod/debuginfod.h > index b4b6a3d2a6b9..53aeee7133ca 100644 > --- a/debuginfod/debuginfod.h > +++ b/debuginfod/debuginfod.h > @@ -1,5 +1,5 @@ > /* External declarations for the libdebuginfod client library. > - Copyright (C) 2019 Red Hat, Inc. > + Copyright (C) 2019-2020 Red Hat, Inc. > This file is part of elfutils. > > This file is free software; you can redistribute it and/or modify > @@ -71,10 +71,23 @@ int debuginfod_find_source (debuginfod_client *client, > const char *filename, > char **path); > > +/* Set a progress callback function. */ > typedef int (*debuginfod_progressfn_t)(debuginfod_client *c, long a, long b); > void debuginfod_set_progressfn(debuginfod_client *c, > debuginfod_progressfn_t fn); > > +/* 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? > +/* 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? > +/* 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"? 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. Thanks, Mark
