From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21617 invoked by alias); 25 Feb 2020 17:07:40 -0000 Mailing-List: contact elfutils-devel-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Post: List-Help: List-Subscribe: Sender: elfutils-devel-owner@sourceware.org Received: (qmail 21607 invoked by uid 89); 25 Feb 2020 17:07:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.100.3 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-6.2 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.1 spammy= X-Spam-Status: No, score=-6.2 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on sourceware.org X-Spam-Level: X-HELO: gnu.wildebeest.org Received: from wildebeest.demon.nl (HELO gnu.wildebeest.org) (212.238.236.112) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 25 Feb 2020 17:07:38 +0000 Received: from tarox.wildebeest.org (tarox.wildebeest.org [172.31.17.39]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id BEF9030008A8; Tue, 25 Feb 2020 18:07:35 +0100 (CET) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id 76A7E4B70B46; Tue, 25 Feb 2020 18:07:35 +0100 (CET) Message-ID: <798d03de900d29cdcbd5fc5d72dea2dc74f5929c.camel@klomp.org> Subject: Re: PR25369 rfc: debuginfod client api extension for progressfn prettying etc. From: Mark Wielaard To: "Frank Ch. Eigler" Cc: elfutils-devel@sourceware.org, simark@simark.ca Date: Tue, 25 Feb 2020 17:07:00 -0000 In-Reply-To: <20200225153202.GB32726@redhat.com> References: <20200225033517.GA28949@redhat.com> <2fef8c25631356b074f19f6da6795c871c24aa9f.camel@klomp.org> <20200225153202.GB32726@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.28.5 (3.28.5-5.el7) Mime-Version: 1.0 X-Spam-Flag: NO X-IsSubscribed: yes X-SW-Source: 2020-q1/txt/msg00142.txt 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 cha= r* header); > >=20 > > 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? >=20 > 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); > >=20 > > 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? >=20 > 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 *valu= e); > > > + > > > +/* Get the user parameter. */ > > > +void* debuginfod_get_user_data (debuginfod_client *client); > >=20 > > 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"? >=20 > 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. >=20 > 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