[PATCH] ceph: initialize pathlen variable in reconnect_caps_cb

[PATCH] ceph: initialize pathlen variable in reconnect_caps_cb

[xiubli]

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>
---
 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;
 
-- 
2.31.1

Re: [PATCH] ceph: initialize pathlen variable in reconnect_caps_cb

[Jeff Layton]

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.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ceph: initialize pathlen variable in reconnect_caps_cb

[Xiubo Li]

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

Re: [PATCH] ceph: initialize pathlen variable in reconnect_caps_cb

[Jeff Layton]

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

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] ceph: initialize pathlen variable in reconnect_caps_cb

[Dan Carpenter]

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