[linux-cifs-client] [PATCH] cifs: cleanup initialization of bytes_written

[Jeff Layton]

On Tue, 30 Mar 2010 18:09:21 +0530
Suresh Jayaraman <sjayaraman at suse.de> wrote:

> On 03/30/2010 05:12 PM, Jeff Layton wrote:
> > On Tue, 30 Mar 2010 14:53:25 +0530
> > Suresh Jayaraman <sjayaraman at suse.de> 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 <sjayaraman at suse.de>
> >> ---
> >>  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.
> 

Agreed.

> > 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.
> 

I think it's generally better to not rely on the callers to get this
right. You've patched them up now, but if someone adds new callers of
these functions later then they also have to get this right. Coding
defensively can help save you from those sorts of bugs.

Since the value should always be overwritten in these functions, it
makes more sense to me to have those functions just zero it out
unconditionally early on. If you think the callers should do this, then
you probably want to add a comment to these functions to make it clear
that the caller is responsible for initializing that variable.

-- 
Jeff Layton <jlayton at samba.org>