On 3/14/19 1:45 AM, David Sterba wrote:
On Wed, Mar 13, 2019 at 06:33:50PM +0800, Anand Jain wrote:
On 3/13/19 1:36 PM, 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@xxxxxxxxxx>
---
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;
Nack.
Now some junk value after expected string is not an error.
such as..
btrfs prop set /btrfs compression lzo110
This was intentional when the zlib levels were introduced so older
kernels can understand newer compression specifier but still have a sane
fallback (ie use the default level). Now it would be better to extend
the validation to parse the method:level format at least. But as
mentioned in the other mail, the level needs to be propagated elsewhere
too so it's not just a change to the validation.
Compression levels properties can be implemented in a newer set of the
patches? So I believe this set ok to merge.
The other way to fix this was to save and rewrite the old parameter,
but that's not a good idea as well, because when parameter is not valid
we should rather fail without any update (generation/transid should
also remain unchanged, there is a bug [1] in the original code which
shall be fixed separately).
[1]
with this patch,
btrfs in dump-super /dev/sdb | grep "^generation"; btrfs prop set
/btrfs compression zli; sync; btrfs in dump-super /dev/sdb | grep
"^generation"
generation 21
ERROR: failed to set compression for /btrfs: Invalid argument
generation 22
Thanks, Anand
