Re: [PATCH v2] btrfs-progs: fi resize: fix false 0.00B sized output

[Su Yue <l@damenly.su>]

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
>>