| Message ID | 20210419130549.148685-1-l@damenly.su (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs-progs: fi resize: fix false 0.00B sized output | expand |
On Mon, Apr 19, 2021 at 09:05:49PM +0800, Su Yue wrote: > Resize to nums without sign prefix makes false output: > btrfs fi resize 1:150g /srv/extra > Resize device id 1 (/dev/sdb1) from 298.09GiB to 0.00B > > The resize operation would take effect though. > > Fix it by handling the case if mod is 0 in check_resize_args(). > > Issue: #307 > Reported-by: Chris Murphy <lists@colorremedies.com> > Signed-off-by: Su Yue <l@damenly.su> Thanks, added to devel.
On Mon, Apr 19, 2021 at 09:05:49PM +0800, Su Yue wrote: > Resize to nums without sign prefix makes false output: > btrfs fi resize 1:150g /srv/extra > Resize device id 1 (/dev/sdb1) from 298.09GiB to 0.00B > > The resize operation would take effect though. > > Fix it by handling the case if mod is 0 in check_resize_args(). > > Issue: #307 > Reported-by: Chris Murphy <lists@colorremedies.com> > Signed-off-by: Su Yue <l@damenly.su> > --- > cmds/filesystem.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > --- > Changelog: > v2: > Calculate u64 diff using max() and min(). > Calculate mod by comparing new and old size. > diff --git a/cmds/filesystem.c b/cmds/filesystem.c > index 9e3cce687d6e..54d46470c53f 100644 > --- a/cmds/filesystem.c > +++ b/cmds/filesystem.c > @@ -1158,6 +1158,16 @@ static int check_resize_args(const char *amount, const char *path) { > } > old_size = di_args[dev_idx].total_bytes; > > + /* For target sizes without '+'/'-' sign prefix(e.g. 1:150g) */ > + if (mod == 0) { > + new_size = diff; > + diff = max(old_size, new_size) - min(old_size, new_size); > + if (new_size > old_size) > + mod = 1; > + else if (new_size < old_size) > + mod = -1; > + } > + > if (mod < 0) { > if (diff > old_size) { > error("current size is %s which is smaller than %s", This fix seems correct to me, but it feels a tiny bit over-complicated. Personally, I think it would be cleaner to do something like: if (mod == 0) { new_size = diff; } else if (mod < 0) { // >0 check new_size = old_size - diff } else { // overflow check new_size = old_size + diff } At this point, new_size is set correctly in all cases and we can do the print. I tested this version on some simple cases, and it seems to work ok as well. Thanks for the fix, Boris > -- > 2.30.1 >
On Mon, Apr 19, 2021 at 10:08:50AM -0700, Boris Burkov wrote: > On Mon, Apr 19, 2021 at 09:05:49PM +0800, Su Yue wrote: > > @@ -1158,6 +1158,16 @@ static int check_resize_args(const char *amount, const char *path) { > > } > > old_size = di_args[dev_idx].total_bytes; > > > > + /* For target sizes without '+'/'-' sign prefix(e.g. 1:150g) */ > > + if (mod == 0) { > > + new_size = diff; > > + diff = max(old_size, new_size) - min(old_size, new_size); > > + if (new_size > old_size) > > + mod = 1; > > + else if (new_size < old_size) > > + mod = -1; > > + } > > + > > if (mod < 0) { > > if (diff > old_size) { > > error("current size is %s which is smaller than %s", > > This fix seems correct to me, but it feels a tiny bit over-complicated. > Personally, I think it would be cleaner to do something like: > > if (mod == 0) { > new_size = diff; > } else if (mod < 0) { > // >0 check > new_size = old_size - diff > } else { > // overflow check > new_size = old_size + diff > } Right, this looks much better and shares a lot of with the code that follows the original fix.
On Tue 20 Apr 2021 at 01:08, Boris Burkov <boris@bur.io> wrote: > On Mon, Apr 19, 2021 at 09:05:49PM +0800, Su Yue wrote: >> Resize to nums without sign prefix makes false output: >> btrfs fi resize 1:150g /srv/extra >> Resize device id 1 (/dev/sdb1) from 298.09GiB to 0.00B >> >> The resize operation would take effect though. >> >> Fix it by handling the case if mod is 0 in check_resize_args(). >> >> Issue: #307 >> Reported-by: Chris Murphy <lists@colorremedies.com> >> Signed-off-by: Su Yue <l@damenly.su> >> --- >> cmds/filesystem.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> --- >> Changelog: >> v2: >> Calculate u64 diff using max() and min(). >> Calculate mod by comparing new and old size. >> diff --git a/cmds/filesystem.c b/cmds/filesystem.c >> index 9e3cce687d6e..54d46470c53f 100644 >> --- a/cmds/filesystem.c >> +++ b/cmds/filesystem.c >> @@ -1158,6 +1158,16 @@ static int check_resize_args(const char >> *amount, const char *path) { >> } >> old_size = di_args[dev_idx].total_bytes; >> >> + /* For target sizes without '+'/'-' sign prefix(e.g. >> 1:150g) */ >> + if (mod == 0) { >> + new_size = diff; >> + diff = max(old_size, new_size) - min(old_size, >> new_size); >> + if (new_size > old_size) >> + mod = 1; >> + else if (new_size < old_size) >> + mod = -1; >> + } >> + >> if (mod < 0) { >> if (diff > old_size) { >> error("current size is %s which is smaller >> than %s", > > This fix seems correct to me, but it feels a tiny bit > over-complicated. > Personally, I think it would be cleaner to do something like: > > if (mod == 0) { > new_size = diff; > } else if (mod < 0) { > // >0 check > new_size = old_size - diff > } else { > // overflow check > new_size = old_size + diff > } > > At this point, new_size is set correctly in all cases and we can > do the > print. I tested this version on some simple cases, and it seems > to work > ok as well. > Right. The first fix when I saw the code was same with yours. Now I relaized that I was over thinking about boundary checks and falling through checks are meaningless if mod is 0. Just to clarify that my fix is wrong: if (mod == 0) { new_size = diff; diff = max(old_size, new_size) - min(old_size, new_size); if (new_size > old_size) mod = 1; else if (new_size < old_size) mod = -1; } /* falls through to check diff */ if (mod < 0) { // new_size < old_size, diff = old_size - new_size // so diff < new_size if (diff > old_size) // can't happen, if mod is 0 before. ... } else if (mod > 0) { // new_size > old_size so diff = new_size(u64) - old_size, if (diff > ULLONG_MAX - old_size) // can't happen even new_size is // ULLONG_MAX if mod is 0 before ... } Thanks for your suggestion, will send v3. -- Su > Thanks for the fix, > Boris > >> -- >> 2.30.1 >>
diff --git a/cmds/filesystem.c b/cmds/filesystem.c index 9e3cce687d6e..54d46470c53f 100644 --- a/cmds/filesystem.c +++ b/cmds/filesystem.c @@ -1158,6 +1158,16 @@ static int check_resize_args(const char *amount, const char *path) { } old_size = di_args[dev_idx].total_bytes; + /* For target sizes without '+'/'-' sign prefix(e.g. 1:150g) */ + if (mod == 0) { + new_size = diff; + diff = max(old_size, new_size) - min(old_size, new_size); + if (new_size > old_size) + mod = 1; + else if (new_size < old_size) + mod = -1; + } + if (mod < 0) { if (diff > old_size) { error("current size is %s which is smaller than %s",
Resize to nums without sign prefix makes false output: btrfs fi resize 1:150g /srv/extra Resize device id 1 (/dev/sdb1) from 298.09GiB to 0.00B The resize operation would take effect though. Fix it by handling the case if mod is 0 in check_resize_args(). Issue: #307 Reported-by: Chris Murphy <lists@colorremedies.com> Signed-off-by: Su Yue <l@damenly.su> --- cmds/filesystem.c | 10 ++++++++++ 1 file changed, 10 insertions(+) --- Changelog: v2: Calculate u64 diff using max() and min(). Calculate mod by comparing new and old size.