[pve-devel] [PATCH storage] ZFSPoolPlugin: fix #2662 get volume size correctly

[Aaron Lauterer]

On 4/3/20 5:07 PM, Fabian Grünbichler wrote:
> there's another instance of 'zfs list ...' in PVE::Storage that could
> also be switched to '-p'

Which one do you mean? Line 610?

   1     if ($format eq 'subvol') { 

610         my $size = $class->zfs_request($scfg, undef, 'list', '-H', 
'-o', 'refquota', "$scfg->{pool}/$basename");
   1         chomp($size);

I don't see much chances for a bug here as the value is used right away 
in the next step but it would definitely be more precise to use the -p flag.
> 
> On April 3, 2020 2:29 pm, Aaron Lauterer wrote:
>> Getting the volume sizes as byte values instead of converted to human
>> readable units helps to avoid rounding errors in the further processing
>> if the volume size is more on the odd side.
>>
>> The `zfs list` command supports the p(arseable) flag since a few years
>> now.
>> When returning the size in bytes there is no  calculation performed and
>> thus we need to explicitly cast the size to an integer before returning
>> it.
>>
>> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
>> ---
>>
>> I don't think we need to worry about other ZFS implementations regarding
>> ZFS over iSCSI. FreeBSD supports it since 9.3 [0], released mid 2014.
>> Illumos added it in 2013 [1].
>>
>> [0] https://www.freebsd.org/cgi/man.cgi?query=zfs&apropos=0&sektion=0&manpath=FreeBSD+9.3-RELEASE&arch=default&format=html
>> [1] https://github.com/illumos/illumos-gate/commit/43d68d68c1ce08fb35026bebfb141af422e7082e#diff-b138320fc5f9d5c48bb4b03a5e4e4cbb
>>
>>   PVE/Storage/ZFSPoolPlugin.pm | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
>> index b538e3b..cb3f2f0 100644
>> --- a/PVE/Storage/ZFSPoolPlugin.pm
>> +++ b/PVE/Storage/ZFSPoolPlugin.pm
>> @@ -81,7 +81,7 @@ sub zfs_parse_size {
>>   	    $size = ceil($size);
>>   	}
> 
> and then the whole zfs_parse_size sub which is completely broken can be
> dropped :)
> 
>>   
>> -	return $size;
>> +	return $size + 0;
> 
> but untainting/converting to int is still needed for those 4 current
> call sites

Will do in V2
> 
>>   
>>       }
>>   
>> @@ -400,7 +400,7 @@ sub zfs_delete_zvol {
>>   sub zfs_list_zvol {
>>       my ($class, $scfg) = @_;
>>   
>> -    my $text = $class->zfs_request($scfg, 10, 'list', '-o', 'name,volsize,origin,type,refquota', '-t', 'volume,filesystem', '-Hr');
>> +    my $text = $class->zfs_request($scfg, 10, 'list', '-o', 'name,volsize,origin,type,refquota', '-t', 'volume,filesystem', '-Hrp');
>>       my $zvols = zfs_parse_zvol_list($text);
>>       return undef if !$zvols;
>>   
>> -- 
>> 2.20.1
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>