[linux-cifs-client] Re: projected date for mount.cifs to support DFS junction points

[Steve French]

On Feb 7, 2008 12:25 PM, Christoph Hellwig <hch at infradead.org> wrote:
> On Wed, Feb 06, 2008 at 07:43:01AM -0600, Steve French wrote:
> > I only remember missing a loop unwinding on exit style comment of
> > yours that was not addressed in what got integrated.  I will go back
> > through your notes again to see if I missed one.
>
>  - there's still all that CONFIG_CIFS_DFS_UPCALL ifdefery left in
>    cifsfs.c that should go into a helper
>  - cifs_fs_type is made non-static but not actually used anywhere
>  - dfs_info3_param still has the camelCase PathConsumed member name
>  - dfs_shrink_umount_helper is called under ifdef instead of a proper
>    stub
>  - dns_resolve.[ch] still have the filename mentioned in the top of file
>    comments
>  - dns_resolve.c still has non-kerneldoc function description comments
>  - dns_resolve.h still has the useless __KERNEL__ ifdef
>  - the unused free_dfs_info_param function is still around
>  - lots of useless and confusing braces left
>  - dns_resolve_server_name_to_ip still has deeply nested conditionals
>    instead of proper goto unwinding
OK - am going through the list.  I thought that Igor had addressed many of
these already.


> There's a reason why we usually repost patches to the list after
> addressing review comments..
AFAIK Igor Mammedov did post the modified DFS patch again (I remember
seeing e.g.
the 2nd or 3rd version of patch 1, the last before it was merged,
posted to the list and
which seemed to include your suggestions, and which did not get any
complaints on fsdevel),
but the additional list you compiled above helps.

> and while I'm at it a lot of the non-DFS additions to cifs aren't quite
> up to standards for kernel code either, lots of useless braces, wierd
> coding style and ifdef mania.
Shaggy made a good suggestion about how to remove the 23
cases of #ifdef CONFIG_CIFS_DEBUG2 (which make the relatively
new cifsacl.c harder to read).  I will post this later today or tomorrow.

> Also running checkpath.pl over them might be a not too bad idea.
I do and I also like checkpatch, it has helped a lot.   In late June
(and then following up a few months later) I did a very large set of
checkpatch corrections for cifs fixing over 80% of the warnings (by
diffing the cifs directory against an empty directory and running the
whole set through checkpatch).   cifs now has a similar number of
checkpatch warnings to other modules its size (and fewer than ext4).
There are two types of checkpatch warnings in particular that I have
not changed.   The cifsacl code (fs/cifs/cifsacl.c) did introduce a
few new checkpatch warnings.

I just ran the diff of the whole cifs directory (against the empty
directory) through the most recent checkpatch and fixed about 40% of
the
remaining cifs checkpatch warnings - most of the rest are not as
important (e.g. the issue discussed earlier about why cifs uses
typedefs in the network protocol definition header file in order to
match the original cifs protocol documentation).


-- 
Thanks,

Steve