From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id A59883858D3C for ; Wed, 8 Dec 2021 15:10:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A59883858D3C 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 tarox.wildebeest.org (83-87-18-245.cable.dynamic.v4.ziggo.nl [83.87.18.245]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 69B3F3000ED0; Wed, 8 Dec 2021 16:10:53 +0100 (CET) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id DC737413CC9A; Wed, 8 Dec 2021 16:10:52 +0100 (CET) Message-ID: <927838d096ccb69450cdc2fc990182689c21373f.camel@klomp.org> Subject: Re: [PATCHv2] debuginfod: Check result of calling MHD_add_response_header. From: Mark Wielaard To: "Frank Ch. Eigler" Cc: elfutils-devel@sourceware.org Date: Wed, 08 Dec 2021 16:10:52 +0100 In-Reply-To: <20211201152301.GF17988@redhat.com> References: <20211201145112.16263-1-mark@klomp.org> <20211201152301.GF17988@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.28.5 (3.28.5-10.el7) Mime-Version: 1.0 X-Spam-Status: No, score=-3.9 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=no 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 Dec 2021 15:10:56 -0000 Hi Frank, On Wed, 2021-12-01 at 10:23 -0500, Frank Ch. Eigler wrote: > > Although unlikely the MHD_add_response_header can fail for > > various reasons. If it fails something odd is going on. > > So check we can actually add a response header and log an > > error if we cannot. >=20 > TBH I wouldn't bother even this much checking. It just uglifies the > code. If it would make covscan happier, I'd stick a (void) in front > of the add-header calls. That is really just like ignoring the issue imho. But you are right that it uglifies the code, I'll wrap the calls in an helper function. > > - MHD_add_response_header (r, "Content-Type", "text/plain"); > > - MHD_RESULT rc =3D MHD_queue_response (c, code, r); > > + MHD_RESULT rc1, rc2; > > + rc1 =3D MHD_add_response_header (r, "Content-Type", > > "text/plain"); > > + rc2 =3D MHD_queue_response (c, code, r); > > MHD_destroy_response (r); > > - return rc; > > + return (rc1 =3D=3D MHD_NO || rc2 =3D=3D MHD_NO) ? MHD_NO : MHD_YES= ; >=20 > e.g. this part won't work: returning MHD_NO causes libmicrohttpd to > send a 503 error back to the caller, regardless of our intended one. OK, so we only want to report MHD_NO here when MHD_queue_response fails. > > + if (MHD_add_response_header (resp, "Last-Modified", > > datebuf) =3D=3D MHD_NO) > > + if (verbose) > > + obatched(clog) << "Error: couldn't add Last-Modified > > header" > > + << endl; > > } >=20 > e.g., we normally report errors to the logs, regardless of verbosity > settings. OK, I'll remove the if (verbose). > > + if (MHD_add_response_header (r, "Content-Type", > > + "application/octet-stream") =3D=3D > > MHD_NO > > + || MHD_add_response_header (r, "X-DEBUGINFOD-SIZE", > > + to_string(s.st_size).c_str() > > ) =3D=3D MHD_NO > > + || MHD_add_response_header (r, "X-DEBUGINFOD-FILE", > > + file.c_str()) =3D=3D MHD_NO) >=20 > e.g., this formulation makes it impossible to add some headers if a > previous one failed. It is likely that if one fails, then all others fail similarly, but I see your point. Any header is better than no headers at all. Thanks, Mark