diff mbox series

[v7,04/19] xfs: Check for -ENOATTR or -EEXIST

Message ID 20200223020611.1802-5-allison.henderson@oracle.com (mailing list archive)
State Superseded, archived
Headers show
Series xfs: Delayed Ready Attrs | expand

Commit Message

Allison Henderson Feb. 23, 2020, 2:05 a.m. UTC 
Delayed operations cannot return error codes.  So we must check for these conditions
first before starting set or remove operations

Signed-off-by: Allison Collins <allison.henderson@oracle.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Amir Goldstein Feb. 23, 2020, 12:25 p.m. UTC | #1 
On Sun, Feb 23, 2020 at 4:07 AM Allison Collins
<allison.henderson@oracle.com> wrote:
>
> Delayed operations cannot return error codes.  So we must check for these conditions
> first before starting set or remove operations
>
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 2255060..a2f812f 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -437,6 +437,14 @@ xfs_attr_set(
>                 goto out_trans_cancel;
>
>         xfs_trans_ijoin(args.trans, dp, 0);
> +
> +       error = xfs_has_attr(&args);
> +       if (error == -EEXIST && (name->type & ATTR_CREATE))
> +               goto out_trans_cancel;
> +
> +       if (error == -ENOATTR && (name->type & ATTR_REPLACE))
> +               goto out_trans_cancel;
> +

And we do care about other errors?

Thanks,
Amir.
Allison Henderson Feb. 23, 2020, 5:33 p.m. UTC | #2 
On 2/23/20 5:25 AM, Amir Goldstein wrote:
> On Sun, Feb 23, 2020 at 4:07 AM Allison Collins
> <allison.henderson@oracle.com> wrote:
>>
>> Delayed operations cannot return error codes.  So we must check for these conditions
>> first before starting set or remove operations
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 2255060..a2f812f 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -437,6 +437,14 @@ xfs_attr_set(
>>                  goto out_trans_cancel;
>>
>>          xfs_trans_ijoin(args.trans, dp, 0);
>> +
>> +       error = xfs_has_attr(&args);
>> +       if (error == -EEXIST && (name->type & ATTR_CREATE))
>> +               goto out_trans_cancel;
>> +
>> +       if (error == -ENOATTR && (name->type & ATTR_REPLACE))
>> +               goto out_trans_cancel;
>> +
> 
> And we do care about other errors?

I guess they would surface during the set operation as they do now, 
though it's probably better to catch it here.  Will add!  Thanks!

Allison

> 
> Thanks,
> Amir.
>
Brian Foster Feb. 24, 2020, 1:08 p.m. UTC | #3 
On Sat, Feb 22, 2020 at 07:05:56PM -0700, Allison Collins wrote:
> Delayed operations cannot return error codes.  So we must check for these conditions
> first before starting set or remove operations
> 
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 2255060..a2f812f 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -437,6 +437,14 @@ xfs_attr_set(
>  		goto out_trans_cancel;
>  
>  	xfs_trans_ijoin(args.trans, dp, 0);
> +
> +	error = xfs_has_attr(&args);
> +	if (error == -EEXIST && (name->type & ATTR_CREATE))
> +		goto out_trans_cancel;
> +
> +	if (error == -ENOATTR && (name->type & ATTR_REPLACE))
> +		goto out_trans_cancel;
> +

So xfs_has_attr() calls the format-specific variant for the attr fork.
If it's node format, xfs_attr_node_hasname() allocs a state and only
frees it if the lookup happens to fail. That looks like a potential
memory leak... Perhaps that helper should free the state in any case the
caller doesn't ask for a pointer?

Brian

>  	error = xfs_attr_set_args(&args);
>  	if (error)
>  		goto out_trans_cancel;
> @@ -525,6 +533,10 @@ xfs_attr_remove(
>  	 */
>  	xfs_trans_ijoin(args.trans, dp, 0);
>  
> +	error = xfs_has_attr(&args);
> +	if (error != -EEXIST)
> +		goto out;
> +
>  	error = xfs_attr_remove_args(&args);
>  	if (error)
>  		goto out;
> -- 
> 2.7.4
>
Allison Henderson Feb. 24, 2020, 9:18 p.m. UTC | #4 
On 2/24/20 6:08 AM, Brian Foster wrote:
> On Sat, Feb 22, 2020 at 07:05:56PM -0700, Allison Collins wrote:
>> Delayed operations cannot return error codes.  So we must check for these conditions
>> first before starting set or remove operations
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 2255060..a2f812f 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -437,6 +437,14 @@ xfs_attr_set(
>>   		goto out_trans_cancel;
>>   
>>   	xfs_trans_ijoin(args.trans, dp, 0);
>> +
>> +	error = xfs_has_attr(&args);
>> +	if (error == -EEXIST && (name->type & ATTR_CREATE))
>> +		goto out_trans_cancel;
>> +
>> +	if (error == -ENOATTR && (name->type & ATTR_REPLACE))
>> +		goto out_trans_cancel;
>> +
> 
> So xfs_has_attr() calls the format-specific variant for the attr fork.
> If it's node format, xfs_attr_node_hasname() allocs a state and only
> frees it if the lookup happens to fail. That looks like a potential
> memory leak... Perhaps that helper should free the state in any case the
> caller doesn't ask for a pointer?
> 
> Brian
Ok, I see it.  Will fix.  Thanks!

Allison


> 
>>   	error = xfs_attr_set_args(&args);
>>   	if (error)
>>   		goto out_trans_cancel;
>> @@ -525,6 +533,10 @@ xfs_attr_remove(
>>   	 */
>>   	xfs_trans_ijoin(args.trans, dp, 0);
>>   
>> +	error = xfs_has_attr(&args);
>> +	if (error != -EEXIST)
>> +		goto out;
>> +
>>   	error = xfs_attr_remove_args(&args);
>>   	if (error)
>>   		goto out;
>> -- 
>> 2.7.4
>>
>
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 2255060..a2f812f 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -437,6 +437,14 @@  xfs_attr_set(
 		goto out_trans_cancel;
 
 	xfs_trans_ijoin(args.trans, dp, 0);
+
+	error = xfs_has_attr(&args);
+	if (error == -EEXIST && (name->type & ATTR_CREATE))
+		goto out_trans_cancel;
+
+	if (error == -ENOATTR && (name->type & ATTR_REPLACE))
+		goto out_trans_cancel;
+
 	error = xfs_attr_set_args(&args);
 	if (error)
 		goto out_trans_cancel;
@@ -525,6 +533,10 @@  xfs_attr_remove(
 	 */
 	xfs_trans_ijoin(args.trans, dp, 0);
 
+	error = xfs_has_attr(&args);
+	if (error != -EEXIST)
+		goto out;
+
 	error = xfs_attr_remove_args(&args);
 	if (error)
 		goto out;