Re: [PATCH 2/14] locks: factor out generic/filesystem switch from test_lock

["J. Bruce Fields" <bfields@fieldses.org>]

On Sat, Feb 03, 2007 at 08:50:25AM +0000, Christoph Hellwig wrote:
> > +int vfs_test_lock(struct file *filp, struct file_lock *fl, struct file_lock *conf)
> 
> Please make sure to add linebreaks after at most 80 characters.

OK, done.

> 
> > +	error = vfs_test_lock(filp, &file_lock, &cfl);
> > +	if (error)
> > +		goto out;
> 
> > +	fl = (cfl.fl_type == F_UNLCK ? NULL : &cfl);
> >  	flock.l_type = F_UNLCK;
> >  	if (fl != NULL) {
> 
> This code snippled is more than ugly.  fl is only checked for equality
> once so you should reformulate that check using the actual type check:
> 
> 	if (cfl.fl_type != F_UNLCK) {
> 
> That also allows you to move the
> 
> 	flock.l_type = fl->fl_type;
> 
> out of the if statement later on.

That's a good idea, thanks; done.

Actually, I wonder if there's any reason we couldn't also just give
posix_test_lock() the same interface as ->lock?  (The latter uses the
same file_lock argument for the input and (in the case where it finds
a conflicting lock) the output, where the former uses an extra argument
to pass back the lock.)  That'd make this a little simpler too.

> In fact that copying out should proably move into posix_lock_to_flock
> and posix_lock_to_fock64 helpers similar to the flock_to_posix_lock
> and flock64_to_posix_lock helpers we have for the other way around.

OK!--b.