diff mbox series

[v19,13/14] xfs: Remove default ASSERT in xfs_attr_set_iter

Message ID 20210525195504.7332-14-allison.henderson@oracle.com (mailing list archive)
State New
Headers show
Series Delay Ready Attributes | expand

Commit Message

Allison Henderson May 25, 2021, 7:55 p.m. UTC
This ASSERT checks for the state value of RM_SHRINK in the set path.
Which would be invalid, and should never happen.  This change is being
set aside from the rest of the set for further discussion

Suggested-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Darrick J. Wong May 25, 2021, 8:52 p.m. UTC | #1
On Tue, May 25, 2021 at 12:55:03PM -0700, Allison Henderson wrote:
> This ASSERT checks for the state value of RM_SHRINK in the set path.
> Which would be invalid, and should never happen.  This change is being
> set aside from the rest of the set for further discussion
> 
> Suggested-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 32d451b..7294a2e 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -612,7 +612,6 @@ xfs_attr_set_iter(
>  		error = xfs_attr_node_addname_clear_incomplete(dac);
>  		break;
>  	default:
> -		ASSERT(dac->dela_state != XFS_DAS_RM_SHRINK);

ASSERT(0); ?

AFAICT the switch statement covers all the states mentioned in the state
diagram for attr setting, so in theory it should be impossible to land
in this state, correct?

--D

>  		break;
>  	}
>  out:
> -- 
> 2.7.4
>
Allison Henderson May 26, 2021, 6:13 p.m. UTC | #2
On 5/25/21 1:52 PM, Darrick J. Wong wrote:
> On Tue, May 25, 2021 at 12:55:03PM -0700, Allison Henderson wrote:
>> This ASSERT checks for the state value of RM_SHRINK in the set path.
>> Which would be invalid, and should never happen.  This change is being
>> set aside from the rest of the set for further discussion
>>
>> Suggested-by: Darrick J. Wong <djwong@kernel.org>
>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 32d451b..7294a2e 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -612,7 +612,6 @@ xfs_attr_set_iter(
>>   		error = xfs_attr_node_addname_clear_incomplete(dac);
>>   		break;
>>   	default:
>> -		ASSERT(dac->dela_state != XFS_DAS_RM_SHRINK);
> 
> ASSERT(0); ?
> 
> AFAICT the switch statement covers all the states mentioned in the state
> diagram for attr setting, so in theory it should be impossible to land
> in this state, correct?
Yes, that's correct, so ASSERT(0); should work too.  I am fine with this 
change if others are.

Allison

> 
> --D
> 
>>   		break;
>>   	}
>>   out:
>> -- 
>> 2.7.4
>>
Chandan Babu R May 27, 2021, 7:40 a.m. UTC | #3
On 26 May 2021 at 23:43, Allison Henderson wrote:
> On 5/25/21 1:52 PM, Darrick J. Wong wrote:
>> On Tue, May 25, 2021 at 12:55:03PM -0700, Allison Henderson wrote:
>>> This ASSERT checks for the state value of RM_SHRINK in the set path.
>>> Which would be invalid, and should never happen.  This change is being
>>> set aside from the rest of the set for further discussion
>>>
>>> Suggested-by: Darrick J. Wong <djwong@kernel.org>
>>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
>>> ---
>>>   fs/xfs/libxfs/xfs_attr.c | 1 -
>>>   1 file changed, 1 deletion(-)
>>>
>>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>>> index 32d451b..7294a2e 100644
>>> --- a/fs/xfs/libxfs/xfs_attr.c
>>> +++ b/fs/xfs/libxfs/xfs_attr.c
>>> @@ -612,7 +612,6 @@ xfs_attr_set_iter(
>>>   		error = xfs_attr_node_addname_clear_incomplete(dac);
>>>   		break;
>>>   	default:
>>> -		ASSERT(dac->dela_state != XFS_DAS_RM_SHRINK);
>>
>> ASSERT(0); ?
>>
>> AFAICT the switch statement covers all the states mentioned in the state
>> diagram for attr setting, so in theory it should be impossible to land
>> in this state, correct?
> Yes, that's correct, so ASSERT(0); should work too.  I am fine with
> this change if others are.

"ASSERT(0);" looks good to me as well.
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 32d451b..7294a2e 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -612,7 +612,6 @@  xfs_attr_set_iter(
 		error = xfs_attr_node_addname_clear_incomplete(dac);
 		break;
 	default:
-		ASSERT(dac->dela_state != XFS_DAS_RM_SHRINK);
 		break;
 	}
 out: