From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (wildebeest.demon.nl [212.238.236.112]) by sourceware.org (Postfix) with ESMTPS id E28B43857817 for ; Wed, 8 Sep 2021 20:56:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E28B43857817 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from reform (deer0x11.wildebeest.org [172.31.17.147]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 805B8301FE93; Wed, 8 Sep 2021 22:56:00 +0200 (CEST) Received: by reform (Postfix, from userid 1000) id 4C40C2E811C5; Wed, 8 Sep 2021 22:56:00 +0200 (CEST) Date: Wed, 8 Sep 2021 22:56:00 +0200 From: Mark Wielaard To: Noah Sanci Cc: elfutils-devel@sourceware.org Subject: Re: [Bug debuginfod/27277] Describe retrieved files when verbose Message-ID: References: <20210805165402.GD4195@redhat.com> <35f2073dfeed8f008d42a78bf60b7efcf13164eb.camel@klomp.org> <20210806185459.GE4195@redhat.com> <9ac621fee207ef233873c40843b3d34ced9019cc.camel@klomp.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Sep 2021 20:56:04 -0000 Hi Noah, Looks like I reviewed a slightly older version earlier. Could you merge the reviewes/changes for both version in a new version of this patch? On Fri, Aug 27, 2021 at 02:38:27PM -0400, Noah Sanci via Elfutils-devel wrote: > From ed7638571f188e346dd466c195b9ebda028d1c65 Mon Sep 17 00:00:00 2001 > From: Noah Sanci > Date: Wed, 28 Jul 2021 14:46:05 -0400 > Subject: [PATCH] debuginfod: PR27277 - Describe retrieved files when verbose > > There appear to exist use cases that intend to simply check for the > existence of content in a debuginfod server, without actually > downloading it. In HTTP land, the HEAD operation is the natural > expression of this. > Instead of implementing a HEAD/describe option, allow users, with enough > verbosity, to print the HTTP response headers upon retrieving a file. Same comment as earlier, but now please also state the debuginfod changes itself (the extra headers added). > E.g output: > > HTTP/1.1 200 OK > Connection: Keep-Alive > Content-Length: 2428240 > Cache-Control: public > Last-Modified: Sat, 15 May 2021 20:49:51 GMT > X-FILE: "file name" > X-FILE-SIZE: "byte length of file" I think an actual output example is better than including "place holder text". > +2021-08-02 Noah Sanci > + > + PR27277 > + * debuginfod-client.c (struct debuginfod_client): New field > + winning_headers. > + (struct handle_data): New field response_data. > + (header_callback): Store received headers in response_data. > + (debuginfod_query_server): Activate CURLOPT_HEADERFUNCTION. > + Save winning response_data. > + (debuginfod_end): free client winning headers. > + * debuginfod.cxx (handle_buildid_f_match): remove X-FILE path. Add > + X-FILE and X-FILE-SIZE headers. > + (handle_buildid_r_match): remove X-FILE path. Add X-FILE, X-FILE-SIZE > + headers, and X-ARCHIVE headers. So the changes compared to the other one are just in the debuginfod.cxx file. > diff --git a/debuginfod/debuginfod.cxx b/debuginfod/debuginfod.cxx > index 6e182a84..e9b7e76c 100644 > --- a/debuginfod/debuginfod.cxx > +++ b/debuginfod/debuginfod.cxx > @@ -1068,6 +1068,9 @@ handle_buildid_f_match (bool internal_req_t, > else > { > MHD_add_response_header (r, "Content-Type", "application/octet-stream"); > + std::string file = b_source0.substr(b_source0.find_last_of("/")+1, b_source0.length()); > + MHD_add_response_header (r, "X-FILE-SIZE", to_string(s.st_size).c_str() ); > + MHD_add_response_header (r, "X-FILE", file.c_str() ); > add_mhd_last_modified (r, s.st_mtime); > if (verbose > 1) > obatched(clog) << "serving file " << b_source0 << endl; > @@ -1537,6 +1540,9 @@ handle_buildid_r_match (bool internal_req_p, > inc_metric ("http_responses_total","result","archive fdcache"); > > MHD_add_response_header (r, "Content-Type", "application/octet-stream"); > + MHD_add_response_header (r, "X-FILE-SIZE", to_string(fs.st_size).c_str()); > + MHD_add_response_header (r, "X-ARCHIVE", b_source0.c_str()); > + MHD_add_response_header (r, "X-FILE", b_source1.c_str()); > add_mhd_last_modified (r, fs.st_mtime); > if (verbose > 1) > obatched(clog) << "serving fdcache archive " << b_source0 << " file " << b_source1 << endl; > @@ -1678,6 +1684,11 @@ handle_buildid_r_match (bool internal_req_p, > else > { > MHD_add_response_header (r, "Content-Type", "application/octet-stream"); > + std::string file = b_source1.substr(b_source1.find_last_of("/")+1, b_source1.length()); > + MHD_add_response_header (r, "X-FILE-SIZE", to_string(fs.st_size).c_str()); > + MHD_add_response_header (r, "X-ARCHIVE", b_source0.c_str()); > + MHD_add_response_header (r, "X-FILE", file.c_str()); > + > add_mhd_last_modified (r, archive_entry_mtime(e)); > if (verbose > 1) > obatched(clog) << "serving archive " << b_source0 << " file " << b_source1 << endl; This looks good. But I wonder if, since these are headers specific to debuginfod, they shouldn't be named X-DEBUGINFOD-... Just to make sure they don't clash with any others a proxy might insert. > +2021-08-04 Noah Sanci > + > + PR27277 > + * debuginfod-find.1: Increasing verbosity describes the downloaded > + file. > + * debuginfod.8: Describe X-FILE, X-FILE-SIZE, and X-ARCHIVE. > + > diff --git a/doc/debuginfod.8 b/doc/debuginfod.8 > index 5b0d793c..e9c58fbb 100644 > --- a/doc/debuginfod.8 > +++ b/doc/debuginfod.8 > @@ -256,6 +256,13 @@ Unknown buildid / request combinations result in HTTP error codes. > This file service resemblance is intentional, so that an installation > can take advantage of standard HTTP management infrastructure. > > +Upon finding a file in an archive or simply in the database, some > +custom http headers are added to the response. For files in the > +database X-FILE and X-FILE-SIZE are added. X-FILE is simply the > +filename and X-FILE-SIZE is the size of the file. For files found > +in archives, in addition to X-FILE and X-FILE-SIZE, X-ARCHIVE is added. > +X-ARCHIVE is the name of the the file was found in. Probably should mention that the file name is unescaped (not url encoded). The last sentence has too many "the". I think a word is missing between the last two. I have again not reviewed the testcase yet. Cheers, Mark