Re: [PATCH] Fix random fast-import errors when compiled with NO_MMAP

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 18, 2008, 06:10 UTC
Message-ID: <7vtzlbirz0.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080118035700.GA3458@spearce.org>

"Shawn O. Pearce" <spearce@spearce.org> writes:

> +extern void close_pack_windows(struct packed_git *, int);

Nobody seems to pass anything but true in retain_fd parameter. Is it worth it?

Show 28 quoted lines

> +void close_pack_windows(struct packed_git *p, int retain_fd)
> +{
> +	struct pack_window *tail_var = NULL, *n = p->windows;
> +	struct pack_window **tail = &tail_var;
> +	while (n) {
> +		struct pack_window *w = p->windows;
> +
> +		if (w->inuse_cnt) {
> +			*tail = w;
> +			tail = &w->next;
> +			continue;
> +		}
> +
> +		munmap(w->base, w->len);
> +		pack_mapped -= w->len;
> +		pack_open_windows--;
> +		n = w->next;
> +		free(w);
> +	}
> +
> +	p->windows = tail_var;
> +	if (p->windows)
> +		warning("pack windows still in-use during close attempt");
> +	else if (!retain_fd && p->pack_fd != -1) {
> +		close(p->pack_fd);
> +		p->pack_fd = -1;
> +	}
> +}

I am not sure about this inuse_cnt business.

The only caller we know that needs this function is fast-import that wants to drop all windows into a pack while keeping the file descriptor and it should not have an in-use windows.

It is unclear what semantics is the right one for callers that do want to retain some windows but still want to call this function. If somebody is in the middle of chasing a delta chain and still calls this function, *why* is the call being made, and what is the right behaviour if not all the windows can be closed because of these open windows?

What about the case the value passed in ratain_fd is 0, which presumably means the caller is asking us to close the file descriptor? If we close the packfile, later accesses through the unclosed windows will obviously barf and I understand that is why you are ignoring retain_fd in that case, but maybe for the caller it was of higher priority that the packfile to get closed than the remaining windows to be still usable. Or maybe the caller wants to be notified of such an error, in which case it probably is not enough to just call warning().

IOW, I think the patch is trying to be too flexible without having a clear definition of what that flexibility is trying to achieve.

Maybe we would need more flexible version later, but I am not convinced if the semantics the above patch implements is the right one.

So I'd prefer something much simpler like this one instead...

void close_pack_windows(struct packed_git *p)
{
	while (p->windows) {
		struct pack_window *w = p->windows;

		if (w->inuse_cnt)
			die("pack '%s' still has outstanding windows",
				p->pack_name)
		munmap(w->base, w->len);
		pack_mapped -= w->len;
		pack_open_windows--;
		p->windows = w->next;
		free(w);
	}
}