Re: [PATCH v5] gc: ignore old gc.log files
On Fri, Feb 10, 2017 at 09:23:15PM +, David Turner wrote: > > Speaking of stderr, I wonder if this function should be calling > > fflush(stderr) before looking at the fstat result. There could be contents > > buffered > > there that haven't been written out yet (not from child processes, but > > perhaps > > ones written in this process itself). > > Probably unlikely in practice, since stderr is typically unbuffered by > > default. > > Process_log_file_at_exit calls fflush. Will fix the other. Ah, good. That makes sense, since we might deadlock if we do it in a signal handler. Perhaps that is a reason not to use stderr here again (though if we want to be that careful, a new fdopen() call is also a bad idea, as we can deadlock over the malloc() lock; you'd have to snprintf to a small buffer and dump it with write()). -Peff
RE: [PATCH v5] gc: ignore old gc.log files
> -Original Message-
> From: Jeff King [mailto:p...@peff.net]
> Sent: Friday, February 10, 2017 4:15 PM
> To: David Turner
> Cc: git@vger.kernel.org; pclo...@gmail.com; Junio C Hamano
>
> Subject: Re: [PATCH v5] gc: ignore old gc.log files
>
> > @@ -76,10 +78,30 @@ static void git_config_date_string(const char
> > *key, const char **output) static void process_log_file(void) {
> > struct stat st;
> > - if (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size)
> > + if (fstat(get_lock_file_fd(&log_lock), &st)) {
> > + /*
> > +* Perhaps there was an i/o error or another
> > +* unlikely situation. Try to make a note of
> > +* this in gc.log along with any existing
> > +* messages.
> > +*/
> > + FILE *fp;
> > + int saved_errno = errno;
> > + fp = fdopen(log_lock.tempfile.fd, "a");
>
> We usually use xfdopen() to handle (unlikely) errors rather than segfaulting.
> But
> I think you'd actually want fdopen_lock_file(), which attaches the fd to the
> tempfile for flushing and cleanup purposes.
>
> That said, I'm not sure I understand why you're opening a new stdio
> filehandle.
> We know that stderr already points to our logfile (that's how content gets
> there
> in the first place). If there's a problem with the file or the descriptor,
> opening a
> new filehandle around the same descriptor won't help.
>
> Speaking of stderr, I wonder if this function should be calling
> fflush(stderr) before looking at the fstat result. There could be contents
> buffered
> there that haven't been written out yet (not from child processes, but perhaps
> ones written in this process itself).
> Probably unlikely in practice, since stderr is typically unbuffered by
> default.
Process_log_file_at_exit calls fflush. Will fix the other.
Re: [PATCH v5] gc: ignore old gc.log files
> @@ -76,10 +78,30 @@ static void git_config_date_string(const char *key, const
> char **output)
> static void process_log_file(void)
> {
> struct stat st;
> - if (!fstat(get_lock_file_fd(&log_lock), &st) && st.st_size)
> + if (fstat(get_lock_file_fd(&log_lock), &st)) {
> + /*
> + * Perhaps there was an i/o error or another
> + * unlikely situation. Try to make a note of
> + * this in gc.log along with any existing
> + * messages.
> + */
> + FILE *fp;
> + int saved_errno = errno;
> + fp = fdopen(log_lock.tempfile.fd, "a");
We usually use xfdopen() to handle (unlikely) errors rather than
segfaulting. But I think you'd actually want fdopen_lock_file(), which
attaches the fd to the tempfile for flushing and cleanup purposes.
That said, I'm not sure I understand why you're opening a new stdio
filehandle. We know that stderr already points to our logfile (that's
how content gets there in the first place). If there's a problem with
the file or the descriptor, opening a new filehandle around the same
descriptor won't help.
Speaking of stderr, I wonder if this function should be calling
fflush(stderr) before looking at the fstat result. There could be
contents buffered there that haven't been written out yet (not from
child processes, but perhaps ones written in this process itself).
Probably unlikely in practice, since stderr is typically unbuffered by
default.
-Peff
