* [PATCH 1/2] btrfs: fix zstd compression parameter
@ 2019-03-13 5:36 Anand Jain
2019-03-13 5:36 ` [PATCH 2/2] btrfs: fix vanished compression property after failed set Anand Jain
2019-03-13 7:19 ` [PATCH 1/2] btrfs: fix zstd compression parameter Nikolay Borisov
0 siblings, 2 replies; 12+ messages in thread
From: Anand Jain @ 2019-03-13 5:36 UTC (permalink / raw)
To: linux-btrfs
We let to pass zstd compression parameter even if its not fully written.
For example:
btrfs prop set /btrfs compression zst
btrfs prop get /btrfs compression
compression=zst
zlib and lzo are fine.
Fix it by using the expected number of char in strncmp().
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
fs/btrfs/props.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 8577ea1d4e2b..ef6502a94712 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -306,7 +306,7 @@ static int prop_compression_apply(struct inode *inode, const char *value,
btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
} else if (!strncmp("zlib", value, 4)) {
type = BTRFS_COMPRESS_ZLIB;
- } else if (!strncmp("zstd", value, len)) {
+ } else if (!strncmp("zstd", value, 4)) {
type = BTRFS_COMPRESS_ZSTD;
btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
} else {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 2/2] btrfs: fix vanished compression property after failed set 2019-03-13 5:36 [PATCH 1/2] btrfs: fix zstd compression parameter Anand Jain @ 2019-03-13 5:36 ` Anand Jain 2019-03-13 7:20 ` Nikolay Borisov 2019-03-13 10:33 ` Anand Jain 2019-03-13 7:19 ` [PATCH 1/2] btrfs: fix zstd compression parameter Nikolay Borisov 1 sibling, 2 replies; 12+ messages in thread From: Anand Jain @ 2019-03-13 5:36 UTC (permalink / raw) To: linux-btrfs 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; return -EINVAL; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] btrfs: fix vanished compression property after failed set 2019-03-13 5:36 ` [PATCH 2/2] btrfs: fix vanished compression property after failed set Anand Jain @ 2019-03-13 7:20 ` Nikolay Borisov 2019-03-13 7:22 ` Nikolay Borisov 2019-03-13 10:33 ` Anand Jain 1 sibling, 1 reply; 12+ messages in thread From: Nikolay Borisov @ 2019-03-13 7:20 UTC (permalink / raw) To: Anand Jain, linux-btrfs 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. > > return -EINVAL; > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] btrfs: fix vanished compression property after failed set 2019-03-13 7:20 ` Nikolay Borisov @ 2019-03-13 7:22 ` Nikolay Borisov 2019-03-13 8:49 ` Anand Jain 0 siblings, 1 reply; 12+ messages in thread From: Nikolay Borisov @ 2019-03-13 7:22 UTC (permalink / raw) To: Anand Jain, linux-btrfs 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. 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. > >> >> return -EINVAL; >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] btrfs: fix vanished compression property after failed set 2019-03-13 7:22 ` Nikolay Borisov @ 2019-03-13 8:49 ` Anand Jain 2019-03-13 17:39 ` David Sterba 0 siblings, 1 reply; 12+ messages in thread From: Anand Jain @ 2019-03-13 8:49 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs 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; >>> >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] btrfs: fix vanished compression property after failed set 2019-03-13 8:49 ` Anand Jain @ 2019-03-13 17:39 ` David Sterba 0 siblings, 0 replies; 12+ messages in thread From: David Sterba @ 2019-03-13 17:39 UTC (permalink / raw) To: Anand Jain; +Cc: Nikolay Borisov, linux-btrfs On Wed, Mar 13, 2019 at 04:49:54PM +0800, Anand Jain wrote: > > > 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. Agreed, some code could be repeated in the apply() callback, but otherwise I'd like to keep both. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] btrfs: fix vanished compression property after failed set 2019-03-13 5:36 ` [PATCH 2/2] btrfs: fix vanished compression property after failed set Anand Jain 2019-03-13 7:20 ` Nikolay Borisov @ 2019-03-13 10:33 ` Anand Jain 2019-03-13 10:49 ` Anand Jain 2019-03-13 17:45 ` David Sterba 1 sibling, 2 replies; 12+ messages in thread From: Anand Jain @ 2019-03-13 10:33 UTC (permalink / raw) To: linux-btrfs 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@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; > Nack. Now some junk value after expected string is not an error. such as.. btrfs prop set /btrfs compression lzo110 Thanks, Anand > return -EINVAL; > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] btrfs: fix vanished compression property after failed set 2019-03-13 10:33 ` Anand Jain @ 2019-03-13 10:49 ` Anand Jain 2019-03-13 17:42 ` David Sterba 2019-03-13 17:45 ` David Sterba 1 sibling, 1 reply; 12+ messages in thread From: Anand Jain @ 2019-03-13 10:49 UTC (permalink / raw) To: linux-btrfs, dsterba On 3/13/19 6:33 PM, 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@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; >> > > > Nack. > Now some junk value after expected string is not an error. > such as.. > > btrfs prop set /btrfs compression lzo110 mount(8) compression and the property compression parameter have diverged, the compression levels are set able only from mount(8) option? We should rather make it consistent? Thanks, Anand > Thanks, Anand > >> return -EINVAL; >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] btrfs: fix vanished compression property after failed set 2019-03-13 10:49 ` Anand Jain @ 2019-03-13 17:42 ` David Sterba 0 siblings, 0 replies; 12+ messages in thread From: David Sterba @ 2019-03-13 17:42 UTC (permalink / raw) To: Anand Jain; +Cc: linux-btrfs, dsterba On Wed, Mar 13, 2019 at 06:49:27PM +0800, Anand Jain wrote: > > > On 3/13/19 6:33 PM, 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@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; > >> > > > > > > Nack. > > Now some junk value after expected string is not an error. > > such as.. > > > > btrfs prop set /btrfs compression lzo110 > > mount(8) compression and the property compression parameter have > diverged, the compression levels are set able only from > mount(8) option? We should rather make it consistent? Yes the properties should be able to get to the same result as the mount option, but this needs more changes as the level is not extracted from the property and passed to the compression (in compress_file_range). ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] btrfs: fix vanished compression property after failed set 2019-03-13 10:33 ` Anand Jain 2019-03-13 10:49 ` Anand Jain @ 2019-03-13 17:45 ` David Sterba 2019-03-14 1:40 ` Anand Jain 1 sibling, 1 reply; 12+ messages in thread From: David Sterba @ 2019-03-13 17:45 UTC (permalink / raw) To: Anand Jain; +Cc: linux-btrfs 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@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; > > > > > 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] btrfs: fix vanished compression property after failed set 2019-03-13 17:45 ` David Sterba @ 2019-03-14 1:40 ` Anand Jain 0 siblings, 0 replies; 12+ messages in thread From: Anand Jain @ 2019-03-14 1:40 UTC (permalink / raw) To: dsterba, linux-btrfs 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@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; >>> >> >> >> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] btrfs: fix zstd compression parameter 2019-03-13 5:36 [PATCH 1/2] btrfs: fix zstd compression parameter Anand Jain 2019-03-13 5:36 ` [PATCH 2/2] btrfs: fix vanished compression property after failed set Anand Jain @ 2019-03-13 7:19 ` Nikolay Borisov 1 sibling, 0 replies; 12+ messages in thread From: Nikolay Borisov @ 2019-03-13 7:19 UTC (permalink / raw) To: Anand Jain, linux-btrfs On 13.03.19 г. 7:36 ч., Anand Jain wrote: > We let to pass zstd compression parameter even if its not fully written. > For example: > btrfs prop set /btrfs compression zst > btrfs prop get /btrfs compression > compression=zst > > zlib and lzo are fine. > > Fix it by using the expected number of char in strncmp(). > > Signed-off-by: Anand Jain <anand.jain@oracle.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/props.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c > index 8577ea1d4e2b..ef6502a94712 100644 > --- a/fs/btrfs/props.c > +++ b/fs/btrfs/props.c > @@ -306,7 +306,7 @@ static int prop_compression_apply(struct inode *inode, const char *value, > btrfs_set_fs_incompat(fs_info, COMPRESS_LZO); > } else if (!strncmp("zlib", value, 4)) { > type = BTRFS_COMPRESS_ZLIB; > - } else if (!strncmp("zstd", value, len)) { > + } else if (!strncmp("zstd", value, 4)) { > type = BTRFS_COMPRESS_ZSTD; > btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD); > } else { > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-03-14 1:40 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-13 5:36 [PATCH 1/2] btrfs: fix zstd compression parameter Anand Jain 2019-03-13 5:36 ` [PATCH 2/2] btrfs: fix vanished compression property after failed set Anand Jain 2019-03-13 7:20 ` Nikolay Borisov 2019-03-13 7:22 ` Nikolay Borisov 2019-03-13 8:49 ` Anand Jain 2019-03-13 17:39 ` David Sterba 2019-03-13 10:33 ` Anand Jain 2019-03-13 10:49 ` Anand Jain 2019-03-13 17:42 ` David Sterba 2019-03-13 17:45 ` David Sterba 2019-03-14 1:40 ` Anand Jain 2019-03-13 7:19 ` [PATCH 1/2] btrfs: fix zstd compression parameter Nikolay Borisov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.