[PATCH][0/6] cifs: readdir.c cleanup

[PATCH][0/6] cifs: readdir.c cleanup

[Jesper Juhl]

Hi Steve,

Here's one more cleanup for a file in fs/cifs - readdir.c (i'm going to 
follow the order you told me you'd prefer first, then do the remaining 
files in arbitrary order).
I'm going to send the patches inline to make it easy for others to comment 
if they so choose, but since you had problems with inline patches from me 
last time I've also placed them online for you :

http://www.linuxtux.org/~juhl/kernel_patches/fs_cifs_readdir-whitespace-cleanup-1.patch
http://www.linuxtux.org/~juhl/kernel_patches/fs_cifs_readdir-whitespace-cleanup-2.patch
http://www.linuxtux.org/~juhl/kernel_patches/fs_cifs_readdir-whitespace-cleanup-3.patch
http://www.linuxtux.org/~juhl/kernel_patches/fs_cifs_readdir-kfree-cleanup.patch
http://www.linuxtux.org/~juhl/kernel_patches/fs_cifs_readdir-cast-cleanup.patch
http://www.linuxtux.org/~juhl/kernel_patches/fs_cifs_readdir-whitespace-cleanup-final-bits.patch

(listed in the order they apply)


Short description of each patch will be in the email with that patch 
inline that will follow shortly.


-- 
Jesper Juhl

Re: [PATCH][0/6] cifs: readdir.c cleanup

[Steve French]

Jesper Juhl wrote:

>Hi Steve,
>
>Here's one more cleanup for a file in fs/cifs - readdir.c (i'm going to 
>follow the order you told me you'd prefer first, then do the remaining 
>files in arbitrary order).
>I'm going to send the patches inline to make it easy for others to comment 
>if they so choose, but since you had problems with inline patches from me 
>last time I've also placed them online for you :
>
>http://www.linuxtux.org/~juhl/kernel_patches/fs_cifs_readdir-whitespace-cleanup-1.patch
>http://www.linuxtux.org/~juhl/kernel_patches/fs_cifs_readdir-whitespace-cleanup-2.patch
>http://www.linuxtux.org/~juhl/kernel_patches/fs_cifs_readdir-whitespace-cleanup-3.patch
>http://www.linuxtux.org/~juhl/kernel_patches/fs_cifs_readdir-kfree-cleanup.patch
>http://www.linuxtux.org/~juhl/kernel_patches/fs_cifs_readdir-cast-cleanup.patch
>http://www.linuxtux.org/~juhl/kernel_patches/fs_cifs_readdir-whitespace-cleanup-final-bits.patch
>
>(listed in the order they apply)
>
>
>Short description of each patch will be in the email with that patch 
>inline that will follow shortly.
>
>
>  
>
The first looks fine.  I am part way through reviewing the second, and 
so far only found one change (see following) that I question.  I prefer 
to keep the local variables together without a blank line between them.  
Is there a global Linux style compliance issue here?  By the way, it is 
not common to use typedefs but you will see a few in this function since 
the network protocol specification describes the format of the wire 
protocol using them and it makes the structure names match the standard.

 static char *nxt_dir_entry(char *old_entry, char *end_of_smb)
 {
-	char * new_entry;
-	FILE_DIRECTORY_INFO * pDirInfo = (FILE_DIRECTORY_INFO *)old_entry;
+	char *new_entry;
+
+	FILE_DIRECTORY_INFO *pDirInfo = (FILE_DIRECTORY_INFO *)old_entry;

I will apply at least a few of them, but I am busy doing a high priority 
fix to handle split transact2 responses (which could cause an oops in ls 
to some servers so is high priority - although it only occurs on large 
directories, and if the server decides to send two transact responses 
for one request (which is not that common) and a search entry is split 
in certain ways across two SMB responses).

Re: [PATCH][0/6] cifs: readdir.c cleanup

[Jesper Juhl]

On Tue, 22 Mar 2005, Steve French wrote:

> Jesper Juhl wrote:
> 
> > Hi Steve,
> > 
> > Here's one more cleanup for a file in fs/cifs - readdir.c (i'm going to
> > follow the order you told me you'd prefer first, then do the remaining files
> > in arbitrary order).
> > I'm going to send the patches inline to make it easy for others to comment
> > if they so choose, but since you had problems with inline patches from me
> > last time I've also placed them online for you :
> > 
[snip]
> >  
> The first looks fine.  I am part way through reviewing the second, and so far
> only found one change (see following) that I question.  I prefer to keep the
> local variables together without a blank line between them.  Is there a global
> Linux style compliance issue here?  By the way, it is not common to use
> typedefs but you will see a few in this function since the network protocol
> specification describes the format of the wire protocol using them and it
> makes the structure names match the standard.
> 
> static char *nxt_dir_entry(char *old_entry, char *end_of_smb)
> {
> -	char * new_entry;
> -	FILE_DIRECTORY_INFO * pDirInfo = (FILE_DIRECTORY_INFO *)old_entry;
> +	char *new_entry;
> +
> +	FILE_DIRECTORY_INFO *pDirInfo = (FILE_DIRECTORY_INFO *)old_entry;
> 
Actually, this is me goofing up. I did intend for all local variables to 
be grouped without a single blank line between them - just one blank line 
*after* the variable block - don't know how this bit snuck in. Thank you 
for catching that.


> I will apply at least a few of them, but I am busy doing a high priority fix
> to handle split transact2 responses (which could cause an oops in ls to some
> servers so is high priority - although it only occurs on large directories,
> and if the server decides to send two transact responses for one request
> (which is not that common) and a search entry is split in certain ways across
> two SMB responses).
> 
I'm fully aware that these patches are low-priority. I don't expect them 
to get anything but "at my convenience" treatment from you.
And should you miss some of them (I'll be sending you quite a few over the 
next few days I expect) I'll just queue them locally and resend at some 
later date (post next Linus release or so), so don't feel pressured to 
look at these if you don't have the time.


-- 
Jesper