[PATCH v3] cachefiles: fix error return code in cachefiles_ondemand_copen()

[PATCH v3] cachefiles: fix error return code in cachefiles_ondemand_copen()

[Sun Ke]

The cache_size field of copen is specified by the user daemon.
If cache_size < 0, then the OPEN request is expected to fail,
while copen itself shall succeed. However, returning 0 is indeed
unexpected when cache_size is an invalid error code.

Fix this by returning error when cache_size is an invalid error code.

Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
Signed-off-by: Sun Ke <sunke32@huawei.com>
---
v3: update the commit log suggested by Jingbo.

 fs/cachefiles/ondemand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index 1fee702d5529..ea8a1e8317d9 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -159,7 +159,7 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
 	/* fail OPEN request if daemon reports an error */
 	if (size < 0) {
 		if (!IS_ERR_VALUE(size))
-			size = -EINVAL;
+			ret = size = -EINVAL;
 		req->error = size;
 		goto out;
 	}
-- 
2.31.1

Re: [Linux-cachefs] [PATCH v3] cachefiles: fix error return code in cachefiles_ondemand_copen()

[Gao Xiang]

On Thu, Aug 18, 2022 at 08:50:38PM +0800, Sun Ke wrote:
> The cache_size field of copen is specified by the user daemon.
> If cache_size < 0, then the OPEN request is expected to fail,
> while copen itself shall succeed. However, returning 0 is indeed
> unexpected when cache_size is an invalid error code.
> 
> Fix this by returning error when cache_size is an invalid error code.
> 
> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
> Signed-off-by: Sun Ke <sunke32@huawei.com>

Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Thanks,
Gao Xiang

> ---
> v3: update the commit log suggested by Jingbo.
> 
>  fs/cachefiles/ondemand.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
> index 1fee702d5529..ea8a1e8317d9 100644
> --- a/fs/cachefiles/ondemand.c
> +++ b/fs/cachefiles/ondemand.c
> @@ -159,7 +159,7 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
>  	/* fail OPEN request if daemon reports an error */
>  	if (size < 0) {
>  		if (!IS_ERR_VALUE(size))
> -			size = -EINVAL;
> +			ret = size = -EINVAL;
>  		req->error = size;
>  		goto out;
>  	}
> -- 
> 2.31.1
> 
> --
> Linux-cachefs mailing list
> Linux-cachefs@redhat.com
> https://listman.redhat.com/mailman/listinfo/linux-cachefs

Re: [PATCH v3] cachefiles: fix error return code in cachefiles_ondemand_copen()

[JeffleXu]

On 8/18/22 8:50 PM, Sun Ke wrote:
> The cache_size field of copen is specified by the user daemon.
> If cache_size < 0, then the OPEN request is expected to fail,
> while copen itself shall succeed. However, returning 0 is indeed
> unexpected when cache_size is an invalid error code.
> 
> Fix this by returning error when cache_size is an invalid error code.
> 
> Fixes: c8383054506c ("cachefiles: notify the user daemon when looking up cookie")
> Signed-off-by: Sun Ke <sunke32@huawei.com>

LGTM.

Reviewed-by: Jingbo Xu <jefflexu@linux.alibaba.com>

> ---
> v3: update the commit log suggested by Jingbo.
> 
>  fs/cachefiles/ondemand.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
> index 1fee702d5529..ea8a1e8317d9 100644
> --- a/fs/cachefiles/ondemand.c
> +++ b/fs/cachefiles/ondemand.c
> @@ -159,7 +159,7 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
>  	/* fail OPEN request if daemon reports an error */
>  	if (size < 0) {
>  		if (!IS_ERR_VALUE(size))
> -			size = -EINVAL;
> +			ret = size = -EINVAL;
>  		req->error = size;
>  		goto out;
>  	}

-- 
Thanks,
Jingbo

Re: [PATCH v3] cachefiles: fix error return code in cachefiles_ondemand_copen()

[David Howells]

	/* fail OPEN request if copen format is invalid */
	ret = kstrtol(psize, 0, &size);
	if (ret) {
		req->error = ret;
		goto out;
	}

	/* fail OPEN request if daemon reports an error */
	if (size < 0) {
		if (!IS_ERR_VALUE(size))
			ret = size = -EINVAL;
		req->error = size;
		goto out;
	}

Should ret get set to the error in size?

David

Re: [PATCH v3] cachefiles: fix error return code in cachefiles_ondemand_copen()

[JeffleXu]

Hi David,

On 8/24/22 6:19 PM, David Howells wrote:
> 	/* fail OPEN request if copen format is invalid */
> 	ret = kstrtol(psize, 0, &size);
> 	if (ret) {
> 		req->error = ret;
> 		goto out;
> 	}
> 
> 	/* fail OPEN request if daemon reports an error */
> 	if (size < 0) {
> 		if (!IS_ERR_VALUE(size))
> 			ret = size = -EINVAL;
> 		req->error = size;
> 		goto out;
> 	}
> 
> Should ret get set to the error in size?


The user daemon completes the OPEN request by replying with the "copen"
command.  The format of "copen" is like: "copen <id>,<cache_size>",
where <cache_size> specifies the size of the backing file. Besides,
<cache_size> is also reused for specifying the error code when the user
daemon thinks it should fail the OPEN request. In this case, the OPEN
request will fail, while the copen command (i.e.
cachefiles_ondemand_copen()) shall return 0, since the format of the
input "copen" command has no problem at all. After all, the error code
inside <cache_size> is specified by the user daemon itself, and the fact
that the OPEN request will fail totally lies in the expectation of the
user daemon.


On the other hand, cachefiles_ondemand_copen() needs to return error
code when the user daemon specifies the "copen" command in a wrong
format, e.g. specifying an invalid error code in <cache_size>. This is
exactly what this patch fixes.


-- 
Thanks,
Jingbo

Re: [PATCH v3] cachefiles: fix error return code in cachefiles_ondemand_copen()

[Dan Carpenter]

I spent a long time looking at this as well...  It's really inscrutable
code.  It would be more readable if we just spelled things out in the
most pedantic way possible:

diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index 1fee702d5529..7e1586bd5cf3 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -158,9 +158,13 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
 
 	/* fail OPEN request if daemon reports an error */
 	if (size < 0) {
-		if (!IS_ERR_VALUE(size))
-			size = -EINVAL;
-		req->error = size;
+		if (!IS_ERR_VALUE(size)) {
+			req->error = -EINVAL;
+			ret = -EINVAL;
+		} else {
+			req->error = size;
+			ret = 0;
+		}
 		goto out;
 	}
 

Re: [Linux-cachefs] [PATCH v3] cachefiles: fix error return code in cachefiles_ondemand_copen()

[Gao Xiang]

On Thu, Aug 25, 2022 at 04:36:20PM +0300, Dan Carpenter wrote:
> I spent a long time looking at this as well...  It's really inscrutable
> code.  It would be more readable if we just spelled things out in the
> most pedantic way possible:
> 

Yeah, the following code looks much better. Ke, would you mind
sending a version like below instead?

Thanks,
Gao Xiang

> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
> index 1fee702d5529..7e1586bd5cf3 100644
> --- a/fs/cachefiles/ondemand.c
> +++ b/fs/cachefiles/ondemand.c
> @@ -158,9 +158,13 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
>  
>  	/* fail OPEN request if daemon reports an error */
>  	if (size < 0) {
> -		if (!IS_ERR_VALUE(size))
> -			size = -EINVAL;
> -		req->error = size;
> +		if (!IS_ERR_VALUE(size)) {
> +			req->error = -EINVAL;
> +			ret = -EINVAL;
> +		} else {
> +			req->error = size;
> +			ret = 0;
> +		}
>  		goto out;
>  	}
>  
> 
> --
> Linux-cachefs mailing list
> Linux-cachefs@redhat.com
> https://listman.redhat.com/mailman/listinfo/linux-cachefs

Re: [Linux-cachefs] [PATCH v3] cachefiles: fix error return code in cachefiles_ondemand_copen()

[Sun Ke]

在 2022/8/25 21:58, Gao Xiang 写道:
> On Thu, Aug 25, 2022 at 04:36:20PM +0300, Dan Carpenter wrote:
>> I spent a long time looking at this as well...  It's really inscrutable
>> code.  It would be more readable if we just spelled things out in the
>> most pedantic way possible:
>>
> 
> Yeah, the following code looks much better. Ke, would you mind
> sending a version like below instead?

OK, I will update it.
> 
> Thanks,
> Gao Xiang
> 
>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>> index 1fee702d5529..7e1586bd5cf3 100644
>> --- a/fs/cachefiles/ondemand.c
>> +++ b/fs/cachefiles/ondemand.c
>> @@ -158,9 +158,13 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
>>   
>>   	/* fail OPEN request if daemon reports an error */
>>   	if (size < 0) {
>> -		if (!IS_ERR_VALUE(size))
>> -			size = -EINVAL;
>> -		req->error = size;
>> +		if (!IS_ERR_VALUE(size)) {
>> +			req->error = -EINVAL;
>> +			ret = -EINVAL;
>> +		} else {
>> +			req->error = size;
>> +			ret = 0;
>> +		}
>>   		goto out;
>>   	}
>>   
>>
>> --
>> Linux-cachefs mailing list
>> Linux-cachefs@redhat.com
>> https://listman.redhat.com/mailman/listinfo/linux-cachefs
> .
>