Web lists-archives.com

Re: [PATCH 1/2] config: check if config path is a file before parsing it




On Fri, Mar 03, 2017 at 05:06:57PM +0700, Duy Nguyen wrote:

> > But if we do, I think we'd either want to:
> >
> >        a. actually check ferror() after getting EOF and report the read
> >           error. That catches EISDIR, along with any other unexpected
> >           errors.
> >
> >        b. use an fopen wrapper that checks fstat(fileno(fh)) after the
> >           open, and turns fopen(some_dir) into an error.
> 
> If you don't like extra check, I guess you're negative on b as well
> since it is an extra check on Windows. That leaves us with option a.

I don't mind _doing_ the extra check that much. I don't think we fopen
so many files that an extra fstat on each would kill us. I mostly just
don't like having to sprinkle the explicit call to it everywhere. I'd be
OK with:

  FILE *xfopen(const char *path, const char *mode)
  {
	FILE *ret = fopen(path, mode);
  #ifdef FOPEN_OPENS_DIRECTORIES
	if (ret) {
		struct stat st;
		if (!fstat(fileno(ret), &st) && S_ISDIR(st.st_mode)) {
			fclose(ret);
			ret = NULL;
		}
	}
  #endif
	return ret;
  }

But I do think option (a) is cleaner. The only trick is that for errno
to be valid, we need to make sure we check ferror() soon after seeing
the EOF return value. I suspect it would work OK in practice for the
git_config_from_file() case.

-Peff