| Message ID | 20211130112034.2711318-1-xiubli@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ceph: initialize pathlen variable in reconnect_caps_cb | expand |
On Tue, 2021-11-30 at 19:20 +0800, xiubli@redhat.com wrote: > From: Xiubo Li <xiubli@redhat.com> > > Silence the potential compiler warning. > > Fixes: a33f6432b3a6 (ceph: encode inodes' parent/d_name in cap reconnect message) > Signed-off-by: Xiubo Li <xiubli@redhat.com> Is this something we need to fix? AFAICT, there is no bug here. In the case where ceph_mdsc_build_path returns an error, "path" will be an ERR_PTR and then ceph_mdsc_free_path will be a no-op. If we do need to take this, we should probably also credit Dan for finding it. > --- > fs/ceph/mds_client.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > index 87f20ed16c6e..2fc2b0a023e4 100644 > --- a/fs/ceph/mds_client.c > +++ b/fs/ceph/mds_client.c > @@ -3711,7 +3711,7 @@ static int reconnect_caps_cb(struct inode *inode, struct ceph_cap *cap, > struct ceph_pagelist *pagelist = recon_state->pagelist; > struct dentry *dentry; > char *path; > - int pathlen, err; > + int pathlen = 0, err; > u64 pathbase; > u64 snap_follows; > If we do take this, you can also get rid of the place where pathlen is set in the !dentry case.
On 11/30/21 8:07 PM, Jeff Layton wrote: > On Tue, 2021-11-30 at 19:20 +0800, xiubli@redhat.com wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> Silence the potential compiler warning. >> >> Fixes: a33f6432b3a6 (ceph: encode inodes' parent/d_name in cap reconnect message) >> Signed-off-by: Xiubo Li <xiubli@redhat.com> > Is this something we need to fix? AFAICT, there is no bug here. > > In the case where ceph_mdsc_build_path returns an error, "path" will be > an ERR_PTR and then ceph_mdsc_free_path will be a no-op. If we do need > to take this, we should probably also credit Dan for finding it. > As I remembered, when I was paying the gluster-block project, the similar cases will always give a warning like this with code sanity checking. >> --- >> fs/ceph/mds_client.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c >> index 87f20ed16c6e..2fc2b0a023e4 100644 >> --- a/fs/ceph/mds_client.c >> +++ b/fs/ceph/mds_client.c >> @@ -3711,7 +3711,7 @@ static int reconnect_caps_cb(struct inode *inode, struct ceph_cap *cap, >> struct ceph_pagelist *pagelist = recon_state->pagelist; >> struct dentry *dentry; >> char *path; >> - int pathlen, err; >> + int pathlen = 0, err; >> u64 pathbase; >> u64 snap_follows; >> > If we do take this, you can also get rid of the place where pathlen is > set in the !dentry case. >
On Tue, 2021-11-30 at 21:12 +0800, Xiubo Li wrote: > On 11/30/21 8:07 PM, Jeff Layton wrote: > > On Tue, 2021-11-30 at 19:20 +0800, xiubli@redhat.com wrote: > > > From: Xiubo Li <xiubli@redhat.com> > > > > > > Silence the potential compiler warning. > > > > > > Fixes: a33f6432b3a6 (ceph: encode inodes' parent/d_name in cap reconnect message) > > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > > Is this something we need to fix? AFAICT, there is no bug here. > > > > In the case where ceph_mdsc_build_path returns an error, "path" will be > > an ERR_PTR and then ceph_mdsc_free_path will be a no-op. If we do need > > to take this, we should probably also credit Dan for finding it. > > > As I remembered, when I was paying the gluster-block project, the > similar cases will always give a warning like this with code sanity > checking. > Meh...ok. I merged a slightly altered version of the patch into the testing branch, revised the changelog and gave Dan reported-by credit. Please take a look when you get a chance and let me know if anything is amiss. Thanks, > > > --- > > > fs/ceph/mds_client.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c > > > index 87f20ed16c6e..2fc2b0a023e4 100644 > > > --- a/fs/ceph/mds_client.c > > > +++ b/fs/ceph/mds_client.c > > > @@ -3711,7 +3711,7 @@ static int reconnect_caps_cb(struct inode *inode, struct ceph_cap *cap, > > > struct ceph_pagelist *pagelist = recon_state->pagelist; > > > struct dentry *dentry; > > > char *path; > > > - int pathlen, err; > > > + int pathlen = 0, err; > > > u64 pathbase; > > > u64 snap_follows; > > > > > If we do take this, you can also get rid of the place where pathlen is > > set in the !dentry case. > > >
On Tue, Nov 30, 2021 at 09:12:42PM +0800, Xiubo Li wrote: > > On 11/30/21 8:07 PM, Jeff Layton wrote: > > On Tue, 2021-11-30 at 19:20 +0800, xiubli@redhat.com wrote: > > > From: Xiubo Li <xiubli@redhat.com> > > > > > > Silence the potential compiler warning. > > > > > > Fixes: a33f6432b3a6 (ceph: encode inodes' parent/d_name in cap reconnect message) > > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > > Is this something we need to fix? AFAICT, there is no bug here. > > > > In the case where ceph_mdsc_build_path returns an error, "path" will be > > an ERR_PTR and then ceph_mdsc_free_path will be a no-op. If we do need > > to take this, we should probably also credit Dan for finding it. > > > As I remembered, when I was paying the gluster-block project, the similar > cases will always give a warning like this with code sanity checking. > I think I was just having a discussion about this. Do you remember what the warnings look like? regards, dan carpenter
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c index 87f20ed16c6e..2fc2b0a023e4 100644 --- a/fs/ceph/mds_client.c +++ b/fs/ceph/mds_client.c @@ -3711,7 +3711,7 @@ static int reconnect_caps_cb(struct inode *inode, struct ceph_cap *cap, struct ceph_pagelist *pagelist = recon_state->pagelist; struct dentry *dentry; char *path; - int pathlen, err; + int pathlen = 0, err; u64 pathbase; u64 snap_follows;