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.
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to