On Sat, Dec 31, 2016 at 04:12:51AM +0100, Michael Haggerty wrote:
> + } else {
> + logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
> if (logfd < 0) {
> - strbuf_addf(err, "unable to append to '%s': %s",
> - logfile->buf, strerror(errno));
> - return -1;
> + if (errno == ENOENT || errno == EISDIR) {
> + /*
> + * The logfile doesn't already exist,
> + * but that is not an error; it only
> + * means that we won't write log
> + * entries to it.
> + */
> + } else {
> + strbuf_addf(err, "unable to append to '%s': %s",
> + logfile->buf, strerror(errno));
> + return -1;
> + }
> }
> }
>
> - adjust_shared_perm(logfile->buf);
> - close(logfd);
> + if (logfd >= 0) {
> + adjust_shared_perm(logfile->buf);
> + close(logfd);
> + }
> +
Hmm. I would have thought in the existing-logfile case that we would not
need to adjust_shared_perm(). But maybe we just do it anyway to pick up
potentially-changed config.
I also had to double-take at this close(). Aren't we calling this
function so we can actually write to the log? But I skipped ahead in
your series and see you address that confusion. :)
-Peff