git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Masaya Suzuki <masayasuzuki@google.com>,
	git@vger.kernel.org, jrnieder@gmail.com, sunshine@sunshineco.com
Subject: Re: [PATCH v2 1/2] Change how HTTP response body is returned
Date: Fri, 4 Jan 2019 05:11:50 -0500
Message-ID: <20190104101149.GA26185@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqqtviph835.fsf@gitster-ct.c.googlers.com>

On Thu, Jan 03, 2019 at 11:09:02AM -0800, Junio C Hamano wrote:

> > +	if (dest->file) {
> > +		/*
> > +		 * At this point, the file contains the response body of the
> > +		 * previous request. We need to truncate the file.
> > +		 */
> > +		FILE *new_file = freopen(dest->filename, "w", dest->file);
> 
> Now freopen() lets us restart the file anew with a new "FILE *".
> 
> > +		if (new_file == NULL) {
> > +			error("Unable to open local file %s", dest->filename);
> 
> error_errno(), perhaps?
> 
> At this point, I presume that dest->file is closed by the failed
> freopen(), but dest->file is still non-NULL and causes further calls
> to http_request() with this dest would be a disaster?  As long as
> the caller of this function reacts to HTTP_ERROR and kill the dest,
> it would be fine.

I also wondered what timing guarantees freopen() gives us (i.e., is it
possible for it to open and truncate the file, and then close the old
handle, flushing some in-memory buffer). C99 says:

  The freopen function first attempts to close any file that is
  associated with the specified stream. Failure to close the file is
  ignored. The error and end-of-file indicators for the stream are
  cleared.

So I think the order is OK for my concern, but not for yours. I.e., on
an error, dest->file is now undefined.

It might be nice to set "dest->file == NULL" in that case. There's no
guarantee that the caller did not hold onto its own copy of the handle,
but since this struct is only exposed internally within http.c, that's
probably OK.

The most robust thing would perhaps be:

  fflush(dest->file);
  ftruncate(fileno(dest->file), 0);

which leaves the handle intact.

(I agree with the rest of your review, especially that it would be
easier to read if this were split into separate refactor and
change-behavior steps).

-Peff

  reply index

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-28  1:47 [PATCH " Masaya Suzuki
2018-12-28  1:47 ` [PATCH 2/2] Unset CURLOPT_FAILONERROR Masaya Suzuki
2018-12-28 19:36   ` Eric Sunshine
2018-12-28 19:51     ` Masaya Suzuki
2018-12-28 19:58       ` Eric Sunshine
2018-12-28 20:00         ` Masaya Suzuki
2018-12-29 19:44 ` [PATCH v2 0/2] Show HTTP headers of failed requests with GIT_CURL_VERBOSE Masaya Suzuki
2018-12-29 19:44   ` [PATCH v2 1/2] Change how HTTP response body is returned Masaya Suzuki
2019-01-03 19:09     ` Junio C Hamano
2019-01-04 10:11       ` Jeff King [this message]
2019-01-04 20:13         ` Junio C Hamano
2019-01-04 10:30     ` Jeff King
2018-12-29 19:44   ` [PATCH v2 2/2] Unset CURLOPT_FAILONERROR Masaya Suzuki
2019-01-04 10:49     ` Jeff King
2019-01-07 23:24       ` Masaya Suzuki
2019-01-08  2:47   ` [PATCH v3 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE Masaya Suzuki
2019-01-08  2:47     ` [PATCH v3 1/5] http: support file handles for HTTP_KEEP_ERROR Masaya Suzuki
2019-01-09 12:15       ` SZEDER Gábor
2019-01-08  2:47     ` [PATCH v3 2/5] http: enable keep_error for HTTP requests Masaya Suzuki
2019-01-08  2:47     ` [PATCH v3 3/5] remote-curl: define struct for CURLOPT_WRITEFUNCTION Masaya Suzuki
2019-01-08  2:47     ` [PATCH v3 4/5] remote-curl: unset CURLOPT_FAILONERROR Masaya Suzuki
2019-01-08  2:47     ` [PATCH v3 5/5] test: test GIT_CURL_VERBOSE=1 shows an error Masaya Suzuki
2019-01-10 19:33     ` [PATCH v4 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE Masaya Suzuki
2019-01-10 19:33       ` [PATCH v4 1/5] http: support file handles for HTTP_KEEP_ERROR Masaya Suzuki
2019-01-10 19:33       ` [PATCH v4 2/5] http: enable keep_error for HTTP requests Masaya Suzuki
2019-01-10 19:33       ` [PATCH v4 3/5] remote-curl: define struct for CURLOPT_WRITEFUNCTION Masaya Suzuki
2019-01-10 19:33       ` [PATCH v4 4/5] remote-curl: unset CURLOPT_FAILONERROR Masaya Suzuki
2019-01-10 19:33       ` [PATCH v4 5/5] test: test GIT_CURL_VERBOSE=1 shows an error Masaya Suzuki
2019-01-10 23:06       ` [PATCH v4 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE Junio C Hamano

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190104101149.GA26185@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=masayasuzuki@google.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox