[prev in list] [next in list] [prev in thread] [next in thread] 

List:       opensolaris-fuse-discuss
Subject:    Re: [fuse-discuss] code for review
From:       Sarah Jelinek <Sarah.Jelinek () Sun ! COM>
Date:       2007-03-29 23:31:50
Message-ID: 460C4C66.1050903 () sun ! com
[Download RAW message or body]

Hi Sunil,

General comments:
-Generating a cscope database for changes would be good. I like to look
around and see how callers, consumers, etc.. of the interfaces are using
them. This helps me to get a fuller picture of what is going on.

-Obviously there is a lot of locking that needs to be added. There is no 
locking on the fuse fs itself during lookup. Stuff like that.

Code comments:
-fuse_create_vnode():
It isn't clear to me why you made this change. I assume the 'iscreate'
is supposed to be for a exclusive create of a non-existent file?

-create_filehandle(): fuse_vnops.c
Is it ever possible that  the msg buff you allocate with
fuse_setup_message() will be NULL or a failure would occur? You don't
check for an error or a null msgp pointer.

-It isn't clear to me why we issue a MKNOD call if the call to
FUSE_CREATE is not supported in the library. Can you explain? Comments
would be helpful as well.

-fuse_msg_refresh(): fuse_vnops.c
You are checking for == size and then > size, can't you check for >= 
size? I am not clear why if it > size it must not be forcefree as well 
to use the same buffer. Can you explain?

-Does fuse_buf_alloc() always return a valid pointer? Is there any error 
checking that needs to happen after a call to this?

-fuse_fs_lookup():fuse_vnops.c
The following:
> /* Check if lookup needs to traverse beyond our filesystem */
>  915         if (nm[0] == '.' && nm[1] == '.' && nm[2] == '\0' &&
>  916             ((fuse_vnode_data_t *)(dvp->v_data))->nodeid == FUSE_ROOT_NODEID) {
>  917                 DEBUG(" .. called from root directory, invoking VOP_LOOKUP \n");
>  918                 err = VOP_LOOKUP(dvp->v_vfsp->vfs_vnodecovered, nm, vpp, pnp,
>  919                     flags, rdir, credp);
>  920                 goto out;
>  921         }

Doesn't check to see if vfs_vnodecovered is NULL. It is possible this 
would be null if the fs was unmounted at the point the lookup was 
called, correct? Because you are calling VOP_LOOKUP on the vnodecovered 
which could be null and no null checking is happening in this function.

-Is it expected that the user level fs implementation will handle the 
pathname parsing and lookup?

-Also, what about symbolic links, is this handled in the user level 
fs?(I would assume so, just checking).

-Why would we create a vnode in the lookup call? How is it possible we 
have a nodeid without an associated vnode in the cache?

thanks,
sarah
****









Sunil wrote:
> Hi all,
> 
> I have implemented following vnode operations for fuse kernel module on 
> Solaris:
> 
> lookup
> create
> readlink
> 
> The webrev generated diff's of the latest changes is available here: 
> http://www-users.cs.umn.edu/~subram/webrev/
> Please feel free to provide any feedback. Depending on the review 
> comments I would expect the changes to be committed in to the repository 
> in a week or two.
> 
> Thanks,
> Sunil
> _______________________________________________
> fuse-discuss mailing list
> fuse-discuss@opensolaris.org
> http://opensolaris.org/mailman/listinfo/fuse-discuss

_______________________________________________
fuse-discuss mailing list
fuse-discuss@opensolaris.org
http://opensolaris.org/mailman/listinfo/fuse-discuss
[prev in list] [next in list] [prev in thread] [next in thread]