On Fri, 28 Mar 2003, Greg Troxel wrote:

>   No. It's coda's issue. From another part of vnode_if.src:
>
>   # For operations other than VOP_LOOKUP which require a component name
>   # parameter, the flags required for the initial namei() call are listed.
>   # Additional flags may be added to the namei() call, but these are required.
>
>   which I take to mean you may get other flags. If you don't want them, be
>   defensive.
>
> I read it otherwise, saying that you have to call namei with some
> flags, but you can put other flags on the namei call if you have some
> reason.  The issue I'm talking about is whether it is ok to have
> random flags, such as LOCKPARENT, set on random vnodeops .  And if so,
> whether they have the described semantics (the notion of LOCKPARENT
> makes sense for VOP_SYMLINK), or are _required to be ignored_ by the
> vnodeop.  Currently, flags like LOCKPARENT appear to be ignored
> (leaving VOP_LOOKUP aside as a special case).

The vnode op should behave as described in vnode_if.src. For symlink, it
is defined to return with the parent unlocked, not with the parent's state
as set by the absense of LOCKPARENT. VOP_LOOKUP is the only call AFAIK
that is defined to respect LOCKPARENT.

> ufs_symlink, for example, does not respect the LOCKPARENT flag; the
> vput is buried deep in ufs_makeinode.  ufs_mkdir is similar.
>
>      The top half of the structure is used exclusively for the
>      pathname lookups using VOP_LOOKUP() and is initialised by the
>      caller.
>
> This is the place where it says that operations other than lookup have
> to ignore the flags.
>
> In the coda_symlink case, LOCKPARENT was on the namei call before
> VOP_SYMLINK, as required.  coda_symlink should ignore LOCKPARENT, and
> thus can't just use the componentname to do a lookup.  The bug is
> arguably in the new coda_symlink code, since VOP_LOOKUP is now called
> with the same componentname structure passed to VOP_SYMLINK.

I agree with the last part - it's coda_symlink's problem.

> The unintended consequences (leaving the parent locked) of reusing
> componentname like this violates the Principle of Least Astonishment,
> and I think it is unclean.  Flags that are defined to be ignored
> shouldn't be set, and perhaps a #ifdef DIAGNOSTIC_EXTRA_PARANOID in

Uhm, that means they aren't being ignored. :-)

> vnode_if.c to check for them.  This issue is quite common in
> kern/vfs_syscalls.c, though.  Arguably it is just plain wrong to call
> lookup reusing a componentname struct one was passed.
>
> The LOCKPARENT issue is purely stylistic; with the &= ~LOCKPARENT in
> coda_symlink I am not aware of any incorrect behavior.
>
>   Cool! Then this is probably the way to go!
>
> I think you are saying that you like the patch to coda_symlink, and
> that sys_symlink is correct.  Committing the coda_symlink patch would
> be nice; the current code is certainly incorrect, although without the
> complication of cfs the bug seems not to be triggered.

I think that the coda_symlink change needs to happen, and that sys_symlink
is not wrong.

Take care,

Bill