Subject: Re: Second patch for lseek() sparse file extension
To: None <reinoud@netbsd.org>
From: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
List: tech-kern
Date: 09/22/2006 11:33:17
> -	switch (SCARG(uap, whence)) {
> -	case SEEK_CUR:
> -		newoff = fp->f_offset + SCARG(uap, offset);
> -		break;
> -	case SEEK_END:
> -		error = VOP_GETATTR(vp, &vattr, cred, l);
> -		if (error)
> -			goto out;
> -		newoff = SCARG(uap, offset) + vattr.va_size;
> -		break;
> -	case SEEK_SET:
> -		newoff = SCARG(uap, offset);
> -		break;
> -	default:
> -		error = EINVAL;
> -		goto out;
> -	}
> -	if ((error = VOP_SEEK(vp, fp->f_offset, newoff, cred)) != 0)
> +	error = VOP_GETATTR(vp, &vattr, cred, l);
> +	if (error)
>  		goto out;
>  
> -	*(off_t *)retval = fp->f_offset = newoff;
> - out:
> +	error = VOP_SEEK(vp, fp->f_offset, SCARG(uap, whence),
> +	    SCARG(uap, offset), &newoffset, &vattr, cred);

passing vattr seems a bad idea to me.
VOP_SEEK (ie. filesystem) itself should have a better idea of
attributes than callers.

>  #
> -# Needs work: Is newoff right?  What's it mean?
>  # XXX Locking protocol?
> +# XXX kauth_cred_t cred is not used
>  #
>  vop_seek {
>  	IN struct vnode *vp;
> -	IN off_t oldoff;
> -	IN off_t newoff;
> +	IN off_t oldoffset;
> +	IN int whence;
> +	IN off_t givenoffset;
> +	OUT off_t *newoffset;
> +	IN struct vattr *vattr;
>  	IN kauth_cred_t cred;
>  };

you need to introduce some locking because you are
introducing a race with block allocation.

YAMAMOTO Takashi