Patchwork cifs: cleanup initialization of bytes_written

login
register
mail settings
Submitter Suresh Jayaraman
Date March 30, 2010, 9:23 a.m.
Message ID <1269941005-17159-1-git-send-email-sjayaraman@suse.de>
Download mbox | patch
Permalink /patch/48946/
State New
Headers show

Comments

Suresh Jayaraman - March 30, 2010, 9:23 a.m.
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@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(-)
Jeff Layton - March 30, 2010, 11:42 a.m.
On Tue, 30 Mar 2010 14:53:25 +0530
Suresh Jayaraman <sjayaraman@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@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
CIFSSMBWrite like Write2 does? Whatever value is in that field should
always be overwritten before those functions return, so I'm not sure I
see the value in pushing the initialization of it out to the callers.
Suresh Jayaraman - March 30, 2010, 12:39 p.m.
On 03/30/2010 05:12 PM, Jeff Layton wrote:
> On Tue, 30 Mar 2010 14:53:25 +0530
> Suresh Jayaraman <sjayaraman@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@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.

> 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,
Jeff Layton - March 30, 2010, 12:50 p.m.
On Tue, 30 Mar 2010 18:09:21 +0530
Suresh Jayaraman <sjayaraman@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@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@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.
Suresh Jayaraman - March 30, 2010, 1:27 p.m.
On 03/30/2010 06:20 PM, Jeff Layton wrote:
> On Tue, 30 Mar 2010 18:09:21 +0530
> Suresh Jayaraman <sjayaraman@suse.de> wrote:
>>
>> 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.

Very valid. Coding defensively makes it less error-prone.

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

Absolutely.


Thanks,

Patch

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,