| Message ID | 1269941005-17159-1-git-send-email-sjayaraman@suse.de |
|---|---|
| State | New |
| Headers | show |
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.
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,
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.
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,
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,
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(-)