Re: [PATCH] cifs: cleanup initialization of bytes_written

On 03/30/2010 05:12 PM, Jeff Layton wrote:
> On Tue, 30 Mar 2010 14:53:25 +0530
> Suresh Jayaraman <[email protected]> wrote:
> 
>> Initialize bytes_written at the callers uniformly so that CIFSSMBWrite2() and
>> CIFSSMBWrite() do not have to worry about it.
>>
>> Signed-off-by: Suresh Jayaraman <[email protected]>
>> ---
>>  fs/cifs/cifssmb.c |    3 ---
>>  fs/cifs/dir.c     |    2 +-
>>  fs/cifs/file.c    |    2 +-
>>  fs/cifs/inode.c   |    4 ++--
>>  4 files changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
>> index 7cc7f83..bbf2afa 100644
>> --- a/fs/cifs/cifssmb.c
>> +++ b/fs/cifs/cifssmb.c
>> @@ -1512,7 +1512,6 @@ CIFSSMBWrite(const int xid, struct cifsTconInfo *tcon,
>>  	cifs_stats_inc(&tcon->num_writes);
>>  	if (rc) {
>>  		cFYI(1, ("Send error in write = %d", rc));
>> -		*nbytes = 0;
>>  	} else {
>>  		*nbytes = le16_to_cpu(pSMBr->CountHigh);
>>  		*nbytes = (*nbytes) << 16;
>> @@ -1539,8 +1538,6 @@ CIFSSMBWrite2(const int xid, struct cifsTconInfo *tcon,
>>  	int smb_hdr_len;
>>  	int resp_buf_type = 0;
>>  
>> -	*nbytes = 0;
>> -
>>  	cFYI(1, ("write2 at %lld %d bytes", (long long)offset, count));
>>  
>>  	if (tcon->ses->capabilities & CAP_LARGE_FILES) {
>> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
>> index e9f7ecc..6d4e7e9 100644
>> --- a/fs/cifs/dir.c
>> +++ b/fs/cifs/dir.c
>> @@ -560,7 +560,7 @@ int cifs_mknod(struct inode *inode, struct dentry *direntry, int mode,
>>  				/* BB Do not bother to decode buf since no
>>  				   local inode yet to put timestamps in,
>>  				   but we can reuse it safely */
>> -				unsigned int bytes_written;
>> +				unsigned int bytes_written = 0;
>>  				struct win_dev *pdev;
>>  				pdev = (struct win_dev *)buf;
>>  				if (S_ISCHR(mode)) {
>> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> index ca2ba7a..6af4fbd 100644
>> --- a/fs/cifs/file.c
>> +++ b/fs/cifs/file.c
>> @@ -1341,7 +1341,7 @@ static int cifs_writepages(struct address_space *mapping,
>>  {
>>  	struct backing_dev_info *bdi = mapping->backing_dev_info;
>>  	unsigned int bytes_to_write;
>> -	unsigned int bytes_written;
>> +	unsigned int bytes_written = 0;
>>  	struct cifs_sb_info *cifs_sb;
>>  	int done = 0;
>>  	pgoff_t end;
>> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> index 723daac..af0b0d4 100644
>> --- a/fs/cifs/inode.c
>> +++ b/fs/cifs/inode.c
>> @@ -1674,7 +1674,7 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>>  		cifsFileInfo_put(open_file);
>>  		cFYI(1, ("SetFSize for attrs rc = %d", rc));
>>  		if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
>> -			unsigned int bytes_written;
>> +			unsigned int bytes_written = 0;
>>  			rc = CIFSSMBWrite(xid, pTcon, nfid, 0, attrs->ia_size,
>>  					  &bytes_written, NULL, NULL, 1);
>>  			cFYI(1, ("Wrt seteof rc %d", rc));
>> @@ -1703,7 +1703,7 @@ cifs_set_file_size(struct inode *inode, struct iattr *attrs,
>>  				cifs_sb->mnt_cifs_flags &
>>  					CIFS_MOUNT_MAP_SPECIAL_CHR);
>>  			if (rc == 0) {
>> -				unsigned int bytes_written;
>> +				unsigned int bytes_written = 0;
>>  				rc = CIFSSMBWrite(xid, pTcon, netfid, 0,
>>  						  attrs->ia_size,
>>  						  &bytes_written, NULL,
> 
> Would it be better to just zero out *nbytes at the beginning of

Might be. I don't have any strong preference to either approach but I
think there should be uniformity and we should remove redundant
initializations.

> CIFSSMBWrite like Write2 does? Whatever value is in that field should
> always be overwritten before those functions return, so I'm not sure I

But my patch fixed the callers because none of these callers assign any
value to bytes_written. The just pass address of bytes_written to
CIFSSMBWRITE calls.

> see the value in pushing the initialization of it out to the callers.
> 

I'll resend this patch if that seems a better approach.

Thanks,

-- 
Suresh Jayaraman