* [PATCH v2 0/5] Fix scan-build warnings
@ 2023-05-03 7:49 Dominique Martinet
2023-05-03 7:49 ` [PATCH v2 1/5] 9p: fix ignored return value in v9fs_dir_release Dominique Martinet
` (5 more replies)
0 siblings, 6 replies; 9+ messages in thread
From: Dominique Martinet @ 2023-05-03 7:49 UTC (permalink / raw)
To: Eric Van Hensbergen, Latchesar Ionkov, Christian Schoenebeck,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Simon Horman, v9fs, linux-kernel, netdev, Dominique Martinet
I ran scan-build very crudly on our source files, and there was at least
one real bug so we might as well run it once in a while, in which case
we probably ought to also fix the less important things hence this
series.
In here the first patch is a real fix and the rest is low priority, the
last one is arguably not an improvement and can be discussed (happy to
just move the 0-initializations around to variable declaration which
would also silence scan-build afaict)
Anyway, it can probably all wait until after this merge, sorry for the
timing.
Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---
Changes in v2:
- Add Fixes tag to first patch (wasn't the one suggested but a very
recent commit, still in -next)
- Fix typo in commit messages
- Link to v1: https://lore.kernel.org/r/20230427-scan-build-v1-0-efa05d65e2da@codewreck.org
---
Dominique Martinet (5):
9p: fix ignored return value in v9fs_dir_release
9p: virtio: fix unlikely null pointer deref in handle_rerror
9p: virtio: make sure 'offs' is initialized in zc_request
9p: virtio: skip incrementing unused variable
9p: remove dead stores (variable set again without being read)
fs/9p/vfs_dir.c | 5 +++--
fs/9p/vfs_inode.c | 6 ------
fs/9p/vfs_inode_dotl.c | 1 -
net/9p/client.c | 46 ++++++++++++----------------------------------
net/9p/trans_virtio.c | 8 ++++----
5 files changed, 19 insertions(+), 47 deletions(-)
---
base-commit: 21e26d5e54ab7cfe6b488fd27d4d70956d07e03b
change-id: 20230427-scan-build-d894c16fc945
Best regards,
--
Dominique Martinet | Asmadeus
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2 1/5] 9p: fix ignored return value in v9fs_dir_release 2023-05-03 7:49 [PATCH v2 0/5] Fix scan-build warnings Dominique Martinet @ 2023-05-03 7:49 ` Dominique Martinet 2023-05-03 7:49 ` [PATCH v2 2/5] 9p: virtio: fix unlikely null pointer deref in handle_rerror Dominique Martinet ` (4 subsequent siblings) 5 siblings, 0 replies; 9+ messages in thread From: Dominique Martinet @ 2023-05-03 7:49 UTC (permalink / raw) To: Eric Van Hensbergen, Latchesar Ionkov, Christian Schoenebeck, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Simon Horman, v9fs, linux-kernel, netdev, Dominique Martinet retval from filemap_fdatawrite was immediately overwritten by the following p9_fid_put: preserve any error in fdatawrite if there was any first. This fixes the following scan-build warning: fs/9p/vfs_dir.c:220:4: warning: Value stored to 'retval' is never read [deadcode.DeadStores] retval = filemap_fdatawrite(inode->i_mapping); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Fixes: d9bc0d11e33b ("fs/9p: Consolidate file operations and add readahead and writeback") Reviewed-by: Simon Horman <simon.horman@corigine.com> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org> --- fs/9p/vfs_dir.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/fs/9p/vfs_dir.c b/fs/9p/vfs_dir.c index 289b58cb896e..54bb99f12390 100644 --- a/fs/9p/vfs_dir.c +++ b/fs/9p/vfs_dir.c @@ -209,7 +209,7 @@ int v9fs_dir_release(struct inode *inode, struct file *filp) struct p9_fid *fid; __le32 version; loff_t i_size; - int retval = 0; + int retval = 0, put_err; fid = filp->private_data; p9_debug(P9_DEBUG_VFS, "inode: %p filp: %p fid: %d\n", @@ -222,7 +222,8 @@ int v9fs_dir_release(struct inode *inode, struct file *filp) spin_lock(&inode->i_lock); hlist_del(&fid->ilist); spin_unlock(&inode->i_lock); - retval = p9_fid_put(fid); + put_err = p9_fid_put(fid); + retval = retval < 0 ? retval : put_err; } if ((filp->f_mode & FMODE_WRITE)) { -- 2.39.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/5] 9p: virtio: fix unlikely null pointer deref in handle_rerror 2023-05-03 7:49 [PATCH v2 0/5] Fix scan-build warnings Dominique Martinet 2023-05-03 7:49 ` [PATCH v2 1/5] 9p: fix ignored return value in v9fs_dir_release Dominique Martinet @ 2023-05-03 7:49 ` Dominique Martinet 2023-05-03 7:49 ` [PATCH v2 3/5] 9p: virtio: make sure 'offs' is initialized in zc_request Dominique Martinet ` (3 subsequent siblings) 5 siblings, 0 replies; 9+ messages in thread From: Dominique Martinet @ 2023-05-03 7:49 UTC (permalink / raw) To: Eric Van Hensbergen, Latchesar Ionkov, Christian Schoenebeck, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Simon Horman, v9fs, linux-kernel, netdev, Dominique Martinet handle_rerror can dereference the pages pointer, but it is not necessarily set for small payloads. In practice these should be filtered out by the size check, but might as well double-check explicitly. This fixes the following scan-build warnings: net/9p/trans_virtio.c:401:24: warning: Dereference of null pointer [core.NullDereference] memcpy_from_page(to, *pages++, offs, n); ^~~~~~~~ net/9p/trans_virtio.c:406:23: warning: Dereference of null pointer (loaded from variable 'pages') [core.NullDereference] memcpy_from_page(to, *pages, offs, size); ^~~~~~ Reviewed-by: Simon Horman <simon.horman@corigine.com> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org> --- net/9p/trans_virtio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c index 3c27ffb781e3..2c9495ccda6b 100644 --- a/net/9p/trans_virtio.c +++ b/net/9p/trans_virtio.c @@ -384,7 +384,7 @@ static void handle_rerror(struct p9_req_t *req, int in_hdr_len, void *to = req->rc.sdata + in_hdr_len; // Fits entirely into the static data? Nothing to do. - if (req->rc.size < in_hdr_len) + if (req->rc.size < in_hdr_len || !pages) return; // Really long error message? Tough, truncate the reply. Might get -- 2.39.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/5] 9p: virtio: make sure 'offs' is initialized in zc_request 2023-05-03 7:49 [PATCH v2 0/5] Fix scan-build warnings Dominique Martinet 2023-05-03 7:49 ` [PATCH v2 1/5] 9p: fix ignored return value in v9fs_dir_release Dominique Martinet 2023-05-03 7:49 ` [PATCH v2 2/5] 9p: virtio: fix unlikely null pointer deref in handle_rerror Dominique Martinet @ 2023-05-03 7:49 ` Dominique Martinet 2023-05-03 7:49 ` [PATCH v2 4/5] 9p: virtio: skip incrementing unused variable Dominique Martinet ` (2 subsequent siblings) 5 siblings, 0 replies; 9+ messages in thread From: Dominique Martinet @ 2023-05-03 7:49 UTC (permalink / raw) To: Eric Van Hensbergen, Latchesar Ionkov, Christian Schoenebeck, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Simon Horman, v9fs, linux-kernel, netdev, Dominique Martinet Similarly to the previous patch: offs can be used in handle_rerrors without initializing on small payloads; in this case handle_rerrors will not use it because of the size check, but it doesn't hurt to make sure it is zero to please scan-build. This fixes the following warning: net/9p/trans_virtio.c:539:3: warning: 3rd function call argument is an uninitialized value [core.CallAndMessage] handle_rerror(req, in_hdr_len, offs, in_pages); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Reviewed-by: Simon Horman <simon.horman@corigine.com> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org> --- net/9p/trans_virtio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c index 2c9495ccda6b..f3f678289423 100644 --- a/net/9p/trans_virtio.c +++ b/net/9p/trans_virtio.c @@ -428,7 +428,7 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req, struct page **in_pages = NULL, **out_pages = NULL; struct virtio_chan *chan = client->trans; struct scatterlist *sgs[4]; - size_t offs; + size_t offs = 0; int need_drop = 0; int kicked = 0; -- 2.39.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 4/5] 9p: virtio: skip incrementing unused variable 2023-05-03 7:49 [PATCH v2 0/5] Fix scan-build warnings Dominique Martinet ` (2 preceding siblings ...) 2023-05-03 7:49 ` [PATCH v2 3/5] 9p: virtio: make sure 'offs' is initialized in zc_request Dominique Martinet @ 2023-05-03 7:49 ` Dominique Martinet 2023-05-03 7:49 ` [PATCH v2 5/5] 9p: remove dead stores (variable set again without being read) Dominique Martinet 2023-07-20 21:26 ` [PATCH v2 0/5] Fix scan-build warnings Eric Van Hensbergen 5 siblings, 0 replies; 9+ messages in thread From: Dominique Martinet @ 2023-05-03 7:49 UTC (permalink / raw) To: Eric Van Hensbergen, Latchesar Ionkov, Christian Schoenebeck, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Simon Horman, v9fs, linux-kernel, netdev, Dominique Martinet Fix the following scan-build warning: net/9p/trans_virtio.c:504:3: warning: Value stored to 'in' is never read [deadcode.DeadStores] in += pack_sg_list_p(chan->sg, out + in, VIRTQUEUE_NUM, ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ I'm honestly not 100% sure about this one; I'm tempted to think we could (should?) just check the return value of pack_sg_list_p to skip the in_sgs++ and setting sgs[] if it didn't process anything, but I'm not sure it should ever happen so this is probably fine as is. Just removing the assignment at least makes it clear the return value isn't used, so it's an improvement in terms of readability. Reviewed-by: Simon Horman <simon.horman@corigine.com> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org> --- net/9p/trans_virtio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c index f3f678289423..e305071eb7b8 100644 --- a/net/9p/trans_virtio.c +++ b/net/9p/trans_virtio.c @@ -501,8 +501,8 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req, if (in_pages) { sgs[out_sgs + in_sgs++] = chan->sg + out + in; - in += pack_sg_list_p(chan->sg, out + in, VIRTQUEUE_NUM, - in_pages, in_nr_pages, offs, inlen); + pack_sg_list_p(chan->sg, out + in, VIRTQUEUE_NUM, + in_pages, in_nr_pages, offs, inlen); } BUG_ON(out_sgs + in_sgs > ARRAY_SIZE(sgs)); -- 2.39.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 5/5] 9p: remove dead stores (variable set again without being read) 2023-05-03 7:49 [PATCH v2 0/5] Fix scan-build warnings Dominique Martinet ` (3 preceding siblings ...) 2023-05-03 7:49 ` [PATCH v2 4/5] 9p: virtio: skip incrementing unused variable Dominique Martinet @ 2023-05-03 7:49 ` Dominique Martinet 2023-05-03 8:22 ` Christian Schoenebeck 2023-07-20 21:26 ` [PATCH v2 0/5] Fix scan-build warnings Eric Van Hensbergen 5 siblings, 1 reply; 9+ messages in thread From: Dominique Martinet @ 2023-05-03 7:49 UTC (permalink / raw) To: Eric Van Hensbergen, Latchesar Ionkov, Christian Schoenebeck, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Simon Horman, v9fs, linux-kernel, netdev, Dominique Martinet The 9p code for some reason used to initialize variables outside of the declaration, e.g. instead of just initializing the variable like this: int retval = 0 We would be doing this: int retval; retval = 0; This is perfectly fine and the compiler will just optimize dead stores anyway, but scan-build seems to think this is a problem and there are many of these warnings making the output of scan-build full of such warnings: fs/9p/vfs_inode.c:916:2: warning: Value stored to 'retval' is never read [deadcode.DeadStores] retval = 0; ^ ~ I have no strong opinion here, but if we want to regularly run scan-build we should fix these just to silence the messages. I've confirmed these all are indeed ok to remove. Reviewed-by: Simon Horman <simon.horman@corigine.com> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org> --- fs/9p/vfs_inode.c | 6 ------ fs/9p/vfs_inode_dotl.c | 1 - net/9p/client.c | 46 ++++++++++++---------------------------------- 3 files changed, 12 insertions(+), 41 deletions(-) diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c index 3791f642c502..99305e97287c 100644 --- a/fs/9p/vfs_inode.c +++ b/fs/9p/vfs_inode.c @@ -164,7 +164,6 @@ int v9fs_uflags2omode(int uflags, int extended) { int ret; - ret = 0; switch (uflags&3) { default: case O_RDONLY: @@ -604,7 +603,6 @@ v9fs_create(struct v9fs_session_info *v9ses, struct inode *dir, p9_debug(P9_DEBUG_VFS, "name %pd\n", dentry); - err = 0; name = dentry->d_name.name; dfid = v9fs_parent_fid(dentry); if (IS_ERR(dfid)) { @@ -816,8 +814,6 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry, if (!(flags & O_CREAT) || d_really_is_positive(dentry)) return finish_no_open(file, res); - err = 0; - v9ses = v9fs_inode2v9ses(dir); perm = unixmode2p9mode(v9ses, mode); p9_omode = v9fs_uflags2omode(flags, v9fs_proto_dotu(v9ses)); @@ -913,7 +909,6 @@ v9fs_vfs_rename(struct mnt_idmap *idmap, struct inode *old_dir, return -EINVAL; p9_debug(P9_DEBUG_VFS, "\n"); - retval = 0; old_inode = d_inode(old_dentry); new_inode = d_inode(new_dentry); v9ses = v9fs_inode2v9ses(old_inode); @@ -1067,7 +1062,6 @@ static int v9fs_vfs_setattr(struct mnt_idmap *idmap, if (retval) return retval; - retval = -EPERM; v9ses = v9fs_dentry2v9ses(dentry); if (iattr->ia_valid & ATTR_FILE) { fid = iattr->ia_file->private_data; diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c index 3acf2bcb69cc..43e282f21962 100644 --- a/fs/9p/vfs_inode_dotl.c +++ b/fs/9p/vfs_inode_dotl.c @@ -367,7 +367,6 @@ static int v9fs_vfs_mkdir_dotl(struct mnt_idmap *idmap, struct posix_acl *dacl = NULL, *pacl = NULL; p9_debug(P9_DEBUG_VFS, "name %pd\n", dentry); - err = 0; v9ses = v9fs_inode2v9ses(dir); omode |= S_IFDIR; diff --git a/net/9p/client.c b/net/9p/client.c index a3340268ec8d..86bbc7147fc1 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -904,7 +904,7 @@ EXPORT_SYMBOL(do_trace_9p_fid_put); static int p9_client_version(struct p9_client *c) { - int err = 0; + int err; struct p9_req_t *req; char *version = NULL; int msize; @@ -975,7 +975,6 @@ struct p9_client *p9_client_create(const char *dev_name, char *options) struct p9_client *clnt; char *client_id; - err = 0; clnt = kmalloc(sizeof(*clnt), GFP_KERNEL); if (!clnt) return ERR_PTR(-ENOMEM); @@ -1094,7 +1093,7 @@ struct p9_fid *p9_client_attach(struct p9_client *clnt, struct p9_fid *afid, const char *uname, kuid_t n_uname, const char *aname) { - int err = 0; + int err; struct p9_req_t *req; struct p9_fid *fid; struct p9_qid qid; @@ -1147,7 +1146,6 @@ struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname, struct p9_req_t *req; u16 nwqids, count; - err = 0; wqids = NULL; clnt = oldfid->clnt; if (clone) { @@ -1224,7 +1222,6 @@ int p9_client_open(struct p9_fid *fid, int mode) clnt = fid->clnt; p9_debug(P9_DEBUG_9P, ">>> %s fid %d mode %d\n", p9_is_proto_dotl(clnt) ? "TLOPEN" : "TOPEN", fid->fid, mode); - err = 0; if (fid->mode != -1) return -EINVAL; @@ -1262,7 +1259,7 @@ EXPORT_SYMBOL(p9_client_open); int p9_client_create_dotl(struct p9_fid *ofid, const char *name, u32 flags, u32 mode, kgid_t gid, struct p9_qid *qid) { - int err = 0; + int err; struct p9_client *clnt; struct p9_req_t *req; int iounit; @@ -1314,7 +1311,6 @@ int p9_client_fcreate(struct p9_fid *fid, const char *name, u32 perm, int mode, p9_debug(P9_DEBUG_9P, ">>> TCREATE fid %d name %s perm %d mode %d\n", fid->fid, name, perm, mode); - err = 0; clnt = fid->clnt; if (fid->mode != -1) @@ -1350,7 +1346,7 @@ EXPORT_SYMBOL(p9_client_fcreate); int p9_client_symlink(struct p9_fid *dfid, const char *name, const char *symtgt, kgid_t gid, struct p9_qid *qid) { - int err = 0; + int err; struct p9_client *clnt; struct p9_req_t *req; @@ -1402,13 +1398,12 @@ EXPORT_SYMBOL(p9_client_link); int p9_client_fsync(struct p9_fid *fid, int datasync) { - int err; + int err = 0; struct p9_client *clnt; struct p9_req_t *req; p9_debug(P9_DEBUG_9P, ">>> TFSYNC fid %d datasync:%d\n", fid->fid, datasync); - err = 0; clnt = fid->clnt; req = p9_client_rpc(clnt, P9_TFSYNC, "dd", fid->fid, datasync); @@ -1428,7 +1423,7 @@ EXPORT_SYMBOL(p9_client_fsync); int p9_client_clunk(struct p9_fid *fid) { - int err; + int err = 0; struct p9_client *clnt; struct p9_req_t *req; int retries = 0; @@ -1436,7 +1431,6 @@ int p9_client_clunk(struct p9_fid *fid) again: p9_debug(P9_DEBUG_9P, ">>> TCLUNK fid %d (try %d)\n", fid->fid, retries); - err = 0; clnt = fid->clnt; req = p9_client_rpc(clnt, P9_TCLUNK, "d", fid->fid); @@ -1465,12 +1459,11 @@ EXPORT_SYMBOL(p9_client_clunk); int p9_client_remove(struct p9_fid *fid) { - int err; + int err = 0; struct p9_client *clnt; struct p9_req_t *req; p9_debug(P9_DEBUG_9P, ">>> TREMOVE fid %d\n", fid->fid); - err = 0; clnt = fid->clnt; req = p9_client_rpc(clnt, P9_TREMOVE, "d", fid->fid); @@ -1680,7 +1673,6 @@ struct p9_wstat *p9_client_stat(struct p9_fid *fid) if (!ret) return ERR_PTR(-ENOMEM); - err = 0; clnt = fid->clnt; req = p9_client_rpc(clnt, P9_TSTAT, "d", fid->fid); @@ -1733,7 +1725,6 @@ struct p9_stat_dotl *p9_client_getattr_dotl(struct p9_fid *fid, if (!ret) return ERR_PTR(-ENOMEM); - err = 0; clnt = fid->clnt; req = p9_client_rpc(clnt, P9_TGETATTR, "dq", fid->fid, request_mask); @@ -1812,11 +1803,10 @@ static int p9_client_statsize(struct p9_wstat *wst, int proto_version) int p9_client_wstat(struct p9_fid *fid, struct p9_wstat *wst) { - int err; + int err = 0; struct p9_req_t *req; struct p9_client *clnt; - err = 0; clnt = fid->clnt; wst->size = p9_client_statsize(wst, clnt->proto_version); p9_debug(P9_DEBUG_9P, ">>> TWSTAT fid %d\n", @@ -1851,11 +1841,10 @@ EXPORT_SYMBOL(p9_client_wstat); int p9_client_setattr(struct p9_fid *fid, struct p9_iattr_dotl *p9attr) { - int err; + int err = 0; struct p9_req_t *req; struct p9_client *clnt; - err = 0; clnt = fid->clnt; p9_debug(P9_DEBUG_9P, ">>> TSETATTR fid %d\n", fid->fid); p9_debug(P9_DEBUG_9P, " valid=%x mode=%x uid=%d gid=%d size=%lld\n", @@ -1887,7 +1876,6 @@ int p9_client_statfs(struct p9_fid *fid, struct p9_rstatfs *sb) struct p9_req_t *req; struct p9_client *clnt; - err = 0; clnt = fid->clnt; p9_debug(P9_DEBUG_9P, ">>> TSTATFS fid %d\n", fid->fid); @@ -1921,11 +1909,10 @@ EXPORT_SYMBOL(p9_client_statfs); int p9_client_rename(struct p9_fid *fid, struct p9_fid *newdirfid, const char *name) { - int err; + int err = 0; struct p9_req_t *req; struct p9_client *clnt; - err = 0; clnt = fid->clnt; p9_debug(P9_DEBUG_9P, ">>> TRENAME fid %d newdirfid %d name %s\n", @@ -1949,11 +1936,10 @@ EXPORT_SYMBOL(p9_client_rename); int p9_client_renameat(struct p9_fid *olddirfid, const char *old_name, struct p9_fid *newdirfid, const char *new_name) { - int err; + int err = 0; struct p9_req_t *req; struct p9_client *clnt; - err = 0; clnt = olddirfid->clnt; p9_debug(P9_DEBUG_9P, @@ -1986,7 +1972,6 @@ struct p9_fid *p9_client_xattrwalk(struct p9_fid *file_fid, struct p9_client *clnt; struct p9_fid *attr_fid; - err = 0; clnt = file_fid->clnt; attr_fid = p9_fid_create(clnt); if (!attr_fid) { @@ -2027,14 +2012,13 @@ EXPORT_SYMBOL_GPL(p9_client_xattrwalk); int p9_client_xattrcreate(struct p9_fid *fid, const char *name, u64 attr_size, int flags) { - int err; + int err = 0; struct p9_req_t *req; struct p9_client *clnt; p9_debug(P9_DEBUG_9P, ">>> TXATTRCREATE fid %d name %s size %llu flag %d\n", fid->fid, name, attr_size, flags); - err = 0; clnt = fid->clnt; req = p9_client_rpc(clnt, P9_TXATTRCREATE, "dsqd", fid->fid, name, attr_size, flags); @@ -2063,7 +2047,6 @@ int p9_client_readdir(struct p9_fid *fid, char *data, u32 count, u64 offset) p9_debug(P9_DEBUG_9P, ">>> TREADDIR fid %d offset %llu count %d\n", fid->fid, offset, count); - err = 0; clnt = fid->clnt; rsize = fid->iounit; @@ -2122,7 +2105,6 @@ int p9_client_mknod_dotl(struct p9_fid *fid, const char *name, int mode, struct p9_client *clnt; struct p9_req_t *req; - err = 0; clnt = fid->clnt; p9_debug(P9_DEBUG_9P, ">>> TMKNOD fid %d name %s mode %d major %d minor %d\n", @@ -2153,7 +2135,6 @@ int p9_client_mkdir_dotl(struct p9_fid *fid, const char *name, int mode, struct p9_client *clnt; struct p9_req_t *req; - err = 0; clnt = fid->clnt; p9_debug(P9_DEBUG_9P, ">>> TMKDIR fid %d name %s mode %d gid %d\n", fid->fid, name, mode, from_kgid(&init_user_ns, gid)); @@ -2182,7 +2163,6 @@ int p9_client_lock_dotl(struct p9_fid *fid, struct p9_flock *flock, u8 *status) struct p9_client *clnt; struct p9_req_t *req; - err = 0; clnt = fid->clnt; p9_debug(P9_DEBUG_9P, ">>> TLOCK fid %d type %i flags %d start %lld length %lld proc_id %d client_id %s\n", @@ -2214,7 +2194,6 @@ int p9_client_getlock_dotl(struct p9_fid *fid, struct p9_getlock *glock) struct p9_client *clnt; struct p9_req_t *req; - err = 0; clnt = fid->clnt; p9_debug(P9_DEBUG_9P, ">>> TGETLOCK fid %d, type %i start %lld length %lld proc_id %d client_id %s\n", @@ -2251,7 +2230,6 @@ int p9_client_readlink(struct p9_fid *fid, char **target) struct p9_client *clnt; struct p9_req_t *req; - err = 0; clnt = fid->clnt; p9_debug(P9_DEBUG_9P, ">>> TREADLINK fid %d\n", fid->fid); -- 2.39.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 5/5] 9p: remove dead stores (variable set again without being read) 2023-05-03 7:49 ` [PATCH v2 5/5] 9p: remove dead stores (variable set again without being read) Dominique Martinet @ 2023-05-03 8:22 ` Christian Schoenebeck 2023-05-03 8:44 ` Dominique Martinet 0 siblings, 1 reply; 9+ messages in thread From: Christian Schoenebeck @ 2023-05-03 8:22 UTC (permalink / raw) To: Eric Van Hensbergen, Latchesar Ionkov, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Dominique Martinet Cc: Simon Horman, v9fs, linux-kernel, netdev, Dominique Martinet On Wednesday, May 3, 2023 9:49:29 AM CEST Dominique Martinet wrote: > The 9p code for some reason used to initialize variables outside of the > declaration, e.g. instead of just initializing the variable like this: > > int retval = 0 > > We would be doing this: > > int retval; > retval = 0; OK, but AFAICS this patch would simply remove all initializations. I would expect at least a default initialization at variable declaration instead. > This is perfectly fine and the compiler will just optimize dead stores > anyway, but scan-build seems to think this is a problem and there are > many of these warnings making the output of scan-build full of such > warnings: > fs/9p/vfs_inode.c:916:2: warning: Value stored to 'retval' is never read [deadcode.DeadStores] > retval = 0; > ^ ~ Honestly I don't see much value in this warning. Can't we just disable this warning for 9p code or is this just controllable for the entire project? Turning variables uninitialized might be fine now, but it also makes the code error prone for future changes. > I have no strong opinion here, but if we want to regularly run > scan-build we should fix these just to silence the messages. > > I've confirmed these all are indeed ok to remove. > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > Signed-off-by: Dominique Martinet <asmadeus@codewreck.org> > --- > fs/9p/vfs_inode.c | 6 ------ > fs/9p/vfs_inode_dotl.c | 1 - > net/9p/client.c | 46 ++++++++++++---------------------------------- > 3 files changed, 12 insertions(+), 41 deletions(-) > > diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c > index 3791f642c502..99305e97287c 100644 > --- a/fs/9p/vfs_inode.c > +++ b/fs/9p/vfs_inode.c > @@ -164,7 +164,6 @@ int v9fs_uflags2omode(int uflags, int extended) > { > int ret; > > - ret = 0; > switch (uflags&3) { > default: > case O_RDONLY: > @@ -604,7 +603,6 @@ v9fs_create(struct v9fs_session_info *v9ses, struct inode *dir, > > p9_debug(P9_DEBUG_VFS, "name %pd\n", dentry); > > - err = 0; > name = dentry->d_name.name; > dfid = v9fs_parent_fid(dentry); > if (IS_ERR(dfid)) { > @@ -816,8 +814,6 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry, > if (!(flags & O_CREAT) || d_really_is_positive(dentry)) > return finish_no_open(file, res); > > - err = 0; > - > v9ses = v9fs_inode2v9ses(dir); > perm = unixmode2p9mode(v9ses, mode); > p9_omode = v9fs_uflags2omode(flags, v9fs_proto_dotu(v9ses)); > @@ -913,7 +909,6 @@ v9fs_vfs_rename(struct mnt_idmap *idmap, struct inode *old_dir, > return -EINVAL; > > p9_debug(P9_DEBUG_VFS, "\n"); > - retval = 0; > old_inode = d_inode(old_dentry); > new_inode = d_inode(new_dentry); > v9ses = v9fs_inode2v9ses(old_inode); > @@ -1067,7 +1062,6 @@ static int v9fs_vfs_setattr(struct mnt_idmap *idmap, > if (retval) > return retval; > > - retval = -EPERM; > v9ses = v9fs_dentry2v9ses(dentry); > if (iattr->ia_valid & ATTR_FILE) { > fid = iattr->ia_file->private_data; > diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c > index 3acf2bcb69cc..43e282f21962 100644 > --- a/fs/9p/vfs_inode_dotl.c > +++ b/fs/9p/vfs_inode_dotl.c > @@ -367,7 +367,6 @@ static int v9fs_vfs_mkdir_dotl(struct mnt_idmap *idmap, > struct posix_acl *dacl = NULL, *pacl = NULL; > > p9_debug(P9_DEBUG_VFS, "name %pd\n", dentry); > - err = 0; > v9ses = v9fs_inode2v9ses(dir); > > omode |= S_IFDIR; > diff --git a/net/9p/client.c b/net/9p/client.c > index a3340268ec8d..86bbc7147fc1 100644 > --- a/net/9p/client.c > +++ b/net/9p/client.c > @@ -904,7 +904,7 @@ EXPORT_SYMBOL(do_trace_9p_fid_put); > > static int p9_client_version(struct p9_client *c) > { > - int err = 0; > + int err; > struct p9_req_t *req; > char *version = NULL; > int msize; > @@ -975,7 +975,6 @@ struct p9_client *p9_client_create(const char *dev_name, char *options) > struct p9_client *clnt; > char *client_id; > > - err = 0; > clnt = kmalloc(sizeof(*clnt), GFP_KERNEL); > if (!clnt) > return ERR_PTR(-ENOMEM); > @@ -1094,7 +1093,7 @@ struct p9_fid *p9_client_attach(struct p9_client *clnt, struct p9_fid *afid, > const char *uname, kuid_t n_uname, > const char *aname) > { > - int err = 0; > + int err; > struct p9_req_t *req; > struct p9_fid *fid; > struct p9_qid qid; > @@ -1147,7 +1146,6 @@ struct p9_fid *p9_client_walk(struct p9_fid *oldfid, uint16_t nwname, > struct p9_req_t *req; > u16 nwqids, count; > > - err = 0; > wqids = NULL; > clnt = oldfid->clnt; > if (clone) { > @@ -1224,7 +1222,6 @@ int p9_client_open(struct p9_fid *fid, int mode) > clnt = fid->clnt; > p9_debug(P9_DEBUG_9P, ">>> %s fid %d mode %d\n", > p9_is_proto_dotl(clnt) ? "TLOPEN" : "TOPEN", fid->fid, mode); > - err = 0; > > if (fid->mode != -1) > return -EINVAL; > @@ -1262,7 +1259,7 @@ EXPORT_SYMBOL(p9_client_open); > int p9_client_create_dotl(struct p9_fid *ofid, const char *name, u32 flags, > u32 mode, kgid_t gid, struct p9_qid *qid) > { > - int err = 0; > + int err; > struct p9_client *clnt; > struct p9_req_t *req; > int iounit; > @@ -1314,7 +1311,6 @@ int p9_client_fcreate(struct p9_fid *fid, const char *name, u32 perm, int mode, > > p9_debug(P9_DEBUG_9P, ">>> TCREATE fid %d name %s perm %d mode %d\n", > fid->fid, name, perm, mode); > - err = 0; > clnt = fid->clnt; > > if (fid->mode != -1) > @@ -1350,7 +1346,7 @@ EXPORT_SYMBOL(p9_client_fcreate); > int p9_client_symlink(struct p9_fid *dfid, const char *name, > const char *symtgt, kgid_t gid, struct p9_qid *qid) > { > - int err = 0; > + int err; > struct p9_client *clnt; > struct p9_req_t *req; > > @@ -1402,13 +1398,12 @@ EXPORT_SYMBOL(p9_client_link); > > int p9_client_fsync(struct p9_fid *fid, int datasync) > { > - int err; > + int err = 0; > struct p9_client *clnt; > struct p9_req_t *req; > > p9_debug(P9_DEBUG_9P, ">>> TFSYNC fid %d datasync:%d\n", > fid->fid, datasync); > - err = 0; > clnt = fid->clnt; > > req = p9_client_rpc(clnt, P9_TFSYNC, "dd", fid->fid, datasync); > @@ -1428,7 +1423,7 @@ EXPORT_SYMBOL(p9_client_fsync); > > int p9_client_clunk(struct p9_fid *fid) > { > - int err; > + int err = 0; > struct p9_client *clnt; > struct p9_req_t *req; > int retries = 0; > @@ -1436,7 +1431,6 @@ int p9_client_clunk(struct p9_fid *fid) > again: > p9_debug(P9_DEBUG_9P, ">>> TCLUNK fid %d (try %d)\n", > fid->fid, retries); > - err = 0; > clnt = fid->clnt; > > req = p9_client_rpc(clnt, P9_TCLUNK, "d", fid->fid); > @@ -1465,12 +1459,11 @@ EXPORT_SYMBOL(p9_client_clunk); > > int p9_client_remove(struct p9_fid *fid) > { > - int err; > + int err = 0; > struct p9_client *clnt; > struct p9_req_t *req; > > p9_debug(P9_DEBUG_9P, ">>> TREMOVE fid %d\n", fid->fid); > - err = 0; > clnt = fid->clnt; > > req = p9_client_rpc(clnt, P9_TREMOVE, "d", fid->fid); > @@ -1680,7 +1673,6 @@ struct p9_wstat *p9_client_stat(struct p9_fid *fid) > if (!ret) > return ERR_PTR(-ENOMEM); > > - err = 0; > clnt = fid->clnt; > > req = p9_client_rpc(clnt, P9_TSTAT, "d", fid->fid); > @@ -1733,7 +1725,6 @@ struct p9_stat_dotl *p9_client_getattr_dotl(struct p9_fid *fid, > if (!ret) > return ERR_PTR(-ENOMEM); > > - err = 0; > clnt = fid->clnt; > > req = p9_client_rpc(clnt, P9_TGETATTR, "dq", fid->fid, request_mask); > @@ -1812,11 +1803,10 @@ static int p9_client_statsize(struct p9_wstat *wst, int proto_version) > > int p9_client_wstat(struct p9_fid *fid, struct p9_wstat *wst) > { > - int err; > + int err = 0; > struct p9_req_t *req; > struct p9_client *clnt; > > - err = 0; > clnt = fid->clnt; > wst->size = p9_client_statsize(wst, clnt->proto_version); > p9_debug(P9_DEBUG_9P, ">>> TWSTAT fid %d\n", > @@ -1851,11 +1841,10 @@ EXPORT_SYMBOL(p9_client_wstat); > > int p9_client_setattr(struct p9_fid *fid, struct p9_iattr_dotl *p9attr) > { > - int err; > + int err = 0; > struct p9_req_t *req; > struct p9_client *clnt; > > - err = 0; > clnt = fid->clnt; > p9_debug(P9_DEBUG_9P, ">>> TSETATTR fid %d\n", fid->fid); > p9_debug(P9_DEBUG_9P, " valid=%x mode=%x uid=%d gid=%d size=%lld\n", > @@ -1887,7 +1876,6 @@ int p9_client_statfs(struct p9_fid *fid, struct p9_rstatfs *sb) > struct p9_req_t *req; > struct p9_client *clnt; > > - err = 0; > clnt = fid->clnt; > > p9_debug(P9_DEBUG_9P, ">>> TSTATFS fid %d\n", fid->fid); > @@ -1921,11 +1909,10 @@ EXPORT_SYMBOL(p9_client_statfs); > int p9_client_rename(struct p9_fid *fid, > struct p9_fid *newdirfid, const char *name) > { > - int err; > + int err = 0; > struct p9_req_t *req; > struct p9_client *clnt; > > - err = 0; > clnt = fid->clnt; > > p9_debug(P9_DEBUG_9P, ">>> TRENAME fid %d newdirfid %d name %s\n", > @@ -1949,11 +1936,10 @@ EXPORT_SYMBOL(p9_client_rename); > int p9_client_renameat(struct p9_fid *olddirfid, const char *old_name, > struct p9_fid *newdirfid, const char *new_name) > { > - int err; > + int err = 0; > struct p9_req_t *req; > struct p9_client *clnt; > > - err = 0; > clnt = olddirfid->clnt; > > p9_debug(P9_DEBUG_9P, > @@ -1986,7 +1972,6 @@ struct p9_fid *p9_client_xattrwalk(struct p9_fid *file_fid, > struct p9_client *clnt; > struct p9_fid *attr_fid; > > - err = 0; > clnt = file_fid->clnt; > attr_fid = p9_fid_create(clnt); > if (!attr_fid) { > @@ -2027,14 +2012,13 @@ EXPORT_SYMBOL_GPL(p9_client_xattrwalk); > int p9_client_xattrcreate(struct p9_fid *fid, const char *name, > u64 attr_size, int flags) > { > - int err; > + int err = 0; > struct p9_req_t *req; > struct p9_client *clnt; > > p9_debug(P9_DEBUG_9P, > ">>> TXATTRCREATE fid %d name %s size %llu flag %d\n", > fid->fid, name, attr_size, flags); > - err = 0; > clnt = fid->clnt; > req = p9_client_rpc(clnt, P9_TXATTRCREATE, "dsqd", > fid->fid, name, attr_size, flags); > @@ -2063,7 +2047,6 @@ int p9_client_readdir(struct p9_fid *fid, char *data, u32 count, u64 offset) > p9_debug(P9_DEBUG_9P, ">>> TREADDIR fid %d offset %llu count %d\n", > fid->fid, offset, count); > > - err = 0; > clnt = fid->clnt; > > rsize = fid->iounit; > @@ -2122,7 +2105,6 @@ int p9_client_mknod_dotl(struct p9_fid *fid, const char *name, int mode, > struct p9_client *clnt; > struct p9_req_t *req; > > - err = 0; > clnt = fid->clnt; > p9_debug(P9_DEBUG_9P, > ">>> TMKNOD fid %d name %s mode %d major %d minor %d\n", > @@ -2153,7 +2135,6 @@ int p9_client_mkdir_dotl(struct p9_fid *fid, const char *name, int mode, > struct p9_client *clnt; > struct p9_req_t *req; > > - err = 0; > clnt = fid->clnt; > p9_debug(P9_DEBUG_9P, ">>> TMKDIR fid %d name %s mode %d gid %d\n", > fid->fid, name, mode, from_kgid(&init_user_ns, gid)); > @@ -2182,7 +2163,6 @@ int p9_client_lock_dotl(struct p9_fid *fid, struct p9_flock *flock, u8 *status) > struct p9_client *clnt; > struct p9_req_t *req; > > - err = 0; > clnt = fid->clnt; > p9_debug(P9_DEBUG_9P, > ">>> TLOCK fid %d type %i flags %d start %lld length %lld proc_id %d client_id %s\n", > @@ -2214,7 +2194,6 @@ int p9_client_getlock_dotl(struct p9_fid *fid, struct p9_getlock *glock) > struct p9_client *clnt; > struct p9_req_t *req; > > - err = 0; > clnt = fid->clnt; > p9_debug(P9_DEBUG_9P, > ">>> TGETLOCK fid %d, type %i start %lld length %lld proc_id %d client_id %s\n", > @@ -2251,7 +2230,6 @@ int p9_client_readlink(struct p9_fid *fid, char **target) > struct p9_client *clnt; > struct p9_req_t *req; > > - err = 0; > clnt = fid->clnt; > p9_debug(P9_DEBUG_9P, ">>> TREADLINK fid %d\n", fid->fid); > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 5/5] 9p: remove dead stores (variable set again without being read) 2023-05-03 8:22 ` Christian Schoenebeck @ 2023-05-03 8:44 ` Dominique Martinet 0 siblings, 0 replies; 9+ messages in thread From: Dominique Martinet @ 2023-05-03 8:44 UTC (permalink / raw) To: Christian Schoenebeck Cc: Eric Van Hensbergen, Latchesar Ionkov, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, v9fs, linux-kernel, netdev Christian Schoenebeck wrote on Wed, May 03, 2023 at 10:22:46AM +0200: > On Wednesday, May 3, 2023 9:49:29 AM CEST Dominique Martinet wrote: > > The 9p code for some reason used to initialize variables outside of the > > declaration, e.g. instead of just initializing the variable like this: > > > > int retval = 0 > > > > We would be doing this: > > > > int retval; > > retval = 0; > > OK, but AFAICS this patch would simply remove all initializations. I would > expect at least a default initialization at variable declaration instead. Yes, clang doesn't seem to complain about 'int reval = 0' so the patch can just be updated to do that instead; I just removed them because the sheer number made it faster to do that. Happy to drop this last patch for now and rework it when time permits. > > This is perfectly fine and the compiler will just optimize dead stores > > anyway, but scan-build seems to think this is a problem and there are > > many of these warnings making the output of scan-build full of such > > warnings: > > fs/9p/vfs_inode.c:916:2: warning: Value stored to 'retval' is never read [deadcode.DeadStores] > > retval = 0; > > ^ ~ > > Honestly I don't see much value in this warning. Can't we just disable this > warning for 9p code or is this just controllable for the entire project? Dead stores in itself is a useful warning, it's what found the real bug where return value was lost in patch 1 of this series, I don't think we should just disable the warning. -- Dominique Martinet | Asmadeus ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 0/5] Fix scan-build warnings 2023-05-03 7:49 [PATCH v2 0/5] Fix scan-build warnings Dominique Martinet ` (4 preceding siblings ...) 2023-05-03 7:49 ` [PATCH v2 5/5] 9p: remove dead stores (variable set again without being read) Dominique Martinet @ 2023-07-20 21:26 ` Eric Van Hensbergen 5 siblings, 0 replies; 9+ messages in thread From: Eric Van Hensbergen @ 2023-07-20 21:26 UTC (permalink / raw) To: Eric Van Hensbergen, Latchesar Ionkov, Christian Schoenebeck, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Dominique Martinet Cc: Simon Horman, v9fs, linux-kernel, netdev On Wed, 03 May 2023 16:49:24 +0900, Dominique Martinet wrote: > I ran scan-build very crudly on our source files, and there was at least > one real bug so we might as well run it once in a while, in which case > we probably ought to also fix the less important things hence this > series. > In here the first patch is a real fix and the rest is low priority, the > last one is arguably not an improvement and can be discussed (happy to > just move the 0-initializations around to variable declaration which > would also silence scan-build afaict) > > [...] Applied, thanks! [1/5] 9p: fix ignored return value in v9fs_dir_release commit: eee4a119e96c2f58cfd1b6d4de42095abc5f8877 [2/5] 9p: virtio: fix unlikely null pointer deref in handle_rerror commit: 13ade4ac5c28e8a014fa85278f5a4270b215f906 [3/5] 9p: virtio: make sure 'offs' is initialized in zc_request commit: 4a73edab69d3a6623f03817fe950a2d9585f80e4 [4/5] 9p: virtio: skip incrementing unused variable commit: f41b402d2572e93bee85669ed05eb5e1f3725704 [5/5] 9p: remove dead stores (variable set again without being read) commit: cf7c33d332ab67603f159123b691c61270b14c33 Best regards, -- Eric Van Hensbergen <ericvh@kernel.org> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-07-20 21:26 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-03 7:49 [PATCH v2 0/5] Fix scan-build warnings Dominique Martinet 2023-05-03 7:49 ` [PATCH v2 1/5] 9p: fix ignored return value in v9fs_dir_release Dominique Martinet 2023-05-03 7:49 ` [PATCH v2 2/5] 9p: virtio: fix unlikely null pointer deref in handle_rerror Dominique Martinet 2023-05-03 7:49 ` [PATCH v2 3/5] 9p: virtio: make sure 'offs' is initialized in zc_request Dominique Martinet 2023-05-03 7:49 ` [PATCH v2 4/5] 9p: virtio: skip incrementing unused variable Dominique Martinet 2023-05-03 7:49 ` [PATCH v2 5/5] 9p: remove dead stores (variable set again without being read) Dominique Martinet 2023-05-03 8:22 ` Christian Schoenebeck 2023-05-03 8:44 ` Dominique Martinet 2023-07-20 21:26 ` [PATCH v2 0/5] Fix scan-build warnings Eric Van Hensbergen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).