Re: [PATCH 2/2] btrfs: fix vanished compression property after failed set

[Anand Jain <anand.jain@oracle.com>]

On 3/13/19 3:22 PM, Nikolay Borisov wrote:
> 
> 
> On 13.03.19 г. 9:20 ч., Nikolay Borisov wrote:
>>
>>
>> On 13.03.19 г. 7:36 ч., Anand Jain wrote:
>>> The compression property resets to NULL, instead of the old value if we
>>> fail to set the new compression parameter.
>>>
>>> btrfs prop get /btrfs compression
>>>    compression=lzo
>>> btrfs prop set /btrfs compression zli
>>>    ERROR: failed to set compression for /btrfs: Invalid argument
>>> btrfs prop get /btrfs compression
>>>
>>> This is because the compression property ->validate() is successful for
>>> 'zli' as the strncmp() used the len passed from the userland.
>>>
>>> Fix it by using the expected string length in strncmp().
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>   fs/btrfs/props.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
>>> index ef6502a94712..7aa362c2fbcf 100644
>>> --- a/fs/btrfs/props.c
>>> +++ b/fs/btrfs/props.c
>>> @@ -277,11 +277,11 @@ static int prop_compression_validate(struct inode *inode, const char *value,
>>>   	if (!value)
>>>   		return 0;
>>>   
>>> -	if (!strncmp("lzo", value, len))
>>> +	if (!strncmp("lzo", value, 3))
>>>   		return 0;
>>> -	else if (!strncmp("zlib", value, len))
>>> +	else if (!strncmp("zlib", value, 4))
>>>   		return 0;
>>> -	else if (!strncmp("zstd", value, len))
>>> +	else if (!strncmp("zstd", value, 4))
>>>   		return 0;
>>
>> This also makes the len argument to prop_compression_validate redundant
>> and should be removed as well.

  Its part of the 'struct prop_handler', its better to keep it until
  properties have completely evolved.

> 
> As a matter of fact I don't see any value in prop_compression_validate
> since the exact same code is used in prop_compression_apply and einval
> will be returned if an invalid value is passed in.

  I notice too. But its better to keep it until the most of the
  properties have evolved.

  As of now btrfs_set_prop() follows sequence..
    h->validate(prop)
    setxattr(prop)
    h->apply(prop)

  If validate fails its easy to fail exit.

Thanks, Anand


>>
>>>   
>>>   	return -EINVAL;
>>>
>>