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

[Mark Wielaard]

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