From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 50062 invoked by alias); 25 Feb 2020 15:25:36 -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 50052 invoked by uid 89); 25 Feb 2020 15:25:36 -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=-18.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.1 spammy= X-Spam-Status: No, score=-18.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,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 15:25:34 +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 B63EC3013319; Tue, 25 Feb 2020 16:25:32 +0100 (CET) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id 4F8634B70B7A; Tue, 25 Feb 2020 16:25:32 +0100 (CET) Message-ID: <2fef8c25631356b074f19f6da6795c871c24aa9f.camel@klomp.org> Subject: Re: PR25369 rfc: debuginfod client api extension for progressfn prettying etc. From: Mark Wielaard To: "Frank Ch. Eigler" , elfutils-devel@sourceware.org Cc: simark@simark.ca Date: Tue, 25 Feb 2020 15:25:00 -0000 In-Reply-To: <20200225033517.GA28949@redhat.com> References: <20200225033517.GA28949@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/msg00139.txt 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=3D1 format message, which now looks like this: >=20 > Downloading from debuginfod / > Downloading from debuginfod - > Downloading from debuginfod \ > Downloading from http://very:8222/buildid/817dcbd2ce0ecce19f22cbe5e136b6d= 9196428aa/executable 433130328/433130328 >=20 > 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. >=20=20 > 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); >=20=20 > +/* Set a progress callback function. */ > typedef int (*debuginfod_progressfn_t)(debuginfod_client *c, long a, lon= g b); > void debuginfod_set_progressfn(debuginfod_client *c, > debuginfod_progressfn_t fn); >=20=20 > +/* Add an outgoing HTTP request "Header: Value". Copies string. */ > +int debuginfod_add_http_header (debuginfod_client *client, const char* h= eader); 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