Re: Unified getcwd() implementation

On Fri, May 07, 2004 at 11:22:35AM +0200, Marc Olzheim wrote:
+> Hi,
+> 
+> (Re: http://lists.freebsd.org/pipermail/freebsd-arch/2003-August/001152.html)
+> 
+> > Yes, it's quite an old patch, and much has happened since it was written.
+> 
+> Mostly some fine-grained locking was introduced.
+> 
+> I hope I got everything covered. Here Bruce's patch reworked, that works
+> for me. (even over NFS ;-))
+> 
+> Anyone care to share their view on it ?

I'm all for it. It will be great to see getcwd() which just work and not
sometimes work. Of course it will be cool to use it in kernel as well.

Some comments:

+> +
+> +	rvp = fdp->fd_rdir;
+> +	if (rvp == NULL)
+> +		rvp = rootvnode;
+> +	VREF(rvp);

I'm not sure here. When fd_rdir could not be set? I hope it is always set
for chrooted/jailed processes.

+> +	error = getcwd_impl(lvp, rvp, &bp, bufp, td);
+> +	if (!error) {
+> +		numcwdfound++;
+> +		if (bufseg == UIO_SYSSPACE)
+> +			bcopy(bp, buf, strlen(bp) + 1);
+> +		else
+> +			error = copyout(bp, buf, strlen(bp) + 1);
+> +	} else {
+> +#if DIAGNOSTIC
+> +		printf("getcwd: error %d\n", error);
+> +#endif
+> +	}

Could we move #if DIAGNOSTIC before '} else {'?
And please, use #ifdef instead of #if.

+> +		CACHE_LOCK();
+> +		ncp = TAILQ_FIRST(&lvp->v_cache_dst);
+> +#if DIAGNOSTIC
+> +		/* XXX simulate cache failure every 10 lookups */
+> +		if ((numcwdcalls % 10) == 0)
[...]
+> +			CACHE_UNLOCK();
+> +			numcwdfail2++;
+> +#if DIAGNOSTIC
+> +			printf("getcwd: using scandir\n");
+> +#endif
+> +			error = getcwd_scandir(&lvp, &uvp, &bp, bufp, td);
+> +			if (error) {
+> +#if DIAGNOSTIC
+> +				printf("getcwd_scandir returned %d\n", error);
+> +#endif
+> +				goto out;
+> +			}
+> +#if DIAGNOSTIC
+> +			if (lvp != NULL)
+> +				panic("getcwd: oops, forgot to null lvp");
+> +#endif
+> +			lvp = uvp;
+> +			uvp = NULL;
+>  		}
+> +#if DIAGNOSTIC
+> +		if (bufp && (bp <= bufp))
+> +			panic("getcwd: oops, went back too far");

s/#if/#ifdef/
And instead of those if+panic I would prefer KASSERT(9), but it is for
INVARIANTS...

I haven't checked/tested this patch, but it looks very promising,
thank you!

-- 
Pawel Jakub Dawidek                       http://www.FreeBSD.org
pjd_at_FreeBSD.org                           http://garage.freebsd.pl
FreeBSD committer                         Am I Evil? Yes, I Am!