[PATCH] [net/9p] Add a Warning to catch NULL fids passed to p9_client_clunk().

[PATCH] [net/9p] Add a Warning to catch NULL fids passed to p9_client_clunk().

[Venkateswararao Jujjuri (JV)]

Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>
---
 net/9p/client.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/net/9p/client.c b/net/9p/client.c
index dc6f2f2..aa7be29 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -1201,6 +1201,12 @@ int p9_client_clunk(struct p9_fid *fid)
 	struct p9_client *clnt;
 	struct p9_req_t *req;
 
+	if (!fid) {
+		P9_EPRINTK(KERN_WARNING, "Trying to clunk with NULL fid\n");
+		dump_stack();
+		return 0;
+	}
+
 	P9_DPRINTK(P9_DEBUG_9P, ">>> TCLUNK fid %d\n", fid->fid);
 	err = 0;
 	clnt = fid->clnt;
-- 
1.6.5.2

Re: [PATCH] [net/9p] Add a Warning to catch NULL fids passed to p9_client_clunk().

[Aneesh Kumar K. V]

On Wed, 25 Aug 2010 09:27:06 -0700, "Venkateswararao Jujjuri (JV)" <jvrao@linux.vnet.ibm.com> wrote:
> Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>
> ---
>  net/9p/client.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/net/9p/client.c b/net/9p/client.c
> index dc6f2f2..aa7be29 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -1201,6 +1201,12 @@ int p9_client_clunk(struct p9_fid *fid)
>  	struct p9_client *clnt;
>  	struct p9_req_t *req;
> 
> +	if (!fid) {
> +		P9_EPRINTK(KERN_WARNING, "Trying to clunk with NULL fid\n");
> +		dump_stack();
> +		return 0;
> +	}
> +
>  	P9_DPRINTK(P9_DEBUG_9P, ">>> TCLUNK fid %d\n", fid->fid);
>  	err = 0;
>  	clnt = fid->clnt;

But why ? We should not have called clunk on null fid. Do you see any
area of code that can do this ? Or is it a debug patch that you did when
developing other features. In case of later do we need to merge this
upstream ?

-aneesh

Re: [PATCH] [net/9p] Add a Warning to catch NULL fids passed to p9_client_clunk().

[Venkateswararao Jujjuri (JV)]

Aneesh Kumar K. V wrote:
> On Wed, 25 Aug 2010 09:27:06 -0700, "Venkateswararao Jujjuri (JV)" <jvrao@linux.vnet.ibm.com> wrote:
>> Signed-off-by: Venkateswararao Jujjuri <jvrao@linux.vnet.ibm.com>
>> ---
>>  net/9p/client.c |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/9p/client.c b/net/9p/client.c
>> index dc6f2f2..aa7be29 100644
>> --- a/net/9p/client.c
>> +++ b/net/9p/client.c
>> @@ -1201,6 +1201,12 @@ int p9_client_clunk(struct p9_fid *fid)
>>  	struct p9_client *clnt;
>>  	struct p9_req_t *req;
>>
>> +	if (!fid) {
>> +		P9_EPRINTK(KERN_WARNING, "Trying to clunk with NULL fid\n");
>> +		dump_stack();
>> +		return 0;
>> +	}
>> +
>>  	P9_DPRINTK(P9_DEBUG_9P, ">>> TCLUNK fid %d\n", fid->fid);
>>  	err = 0;
>>  	clnt = fid->clnt;
> 
> But why ? We should not have called clunk on null fid. Do you see any
> area of code that can do this ? Or is it a debug patch that you did when
> developing other features. In case of later do we need to merge this
> upstream ?

It is kind of debug patch..as we don't know any known case, assuming that the
v9fs_dir_release()
goes into mainline.
It will be nice if merged into upstream..but not an absolute requirement.

Thanks,
JV

> 
> -aneesh