[pve-devel] [PATCH storage 2/5] Diskmanage: allow get_disks to take multiple disks

[Dominik Csapak]

On 6/4/19 10:08 AM, Thomas Lamprecht wrote:
> On 6/4/19 9:21 AM, Dominik Csapak wrote:
>> we now expect the first parameter to be either a string with a single
>> disk, or an array ref with a list of disks
>>
>> this way we can get the info of multiple disks simultaneously while
>> not iterating over all disks
>>
>> this will be used to get the info for osd/db/wal disk
>>
>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>> ---
>>   PVE/Diskmanage.pm | 22 ++++++++++++++++------
>>   1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
>> index a7a7e70..daad6df 100644
>> --- a/PVE/Diskmanage.pm
>> +++ b/PVE/Diskmanage.pm
>> @@ -416,7 +416,7 @@ sub is_iscsi {
>>   }
>>   
>>   sub get_disks {
>> -    my ($disk, $nosmart) = @_;
>> +    my ($disks, $nosmart) = @_;
>>       my $disklist = {};
>>   
>>       my $mounted = {};
>> @@ -440,14 +440,24 @@ sub get_disks {
>>   
>>       my $lvmlist = get_lvm_devices();
>>   
>> -    # we get cciss/c0d0 but need cciss!c0d0
>> -    if (defined($disk) && $disk =~ m|^cciss/|) {
>> -	$disk =~ s|cciss/|cciss!|;
>> +    my $path = ".*";
> 
> please call this something like:
> $disk_re, $disks_regex or $disk_filter
> 
> to make it clearer as ".*" is a strange path

ok

> 
>> +    if (defined($disks)) {
>> +	if (!ref($disks) || ref($disks) ne 'ARRAY') {
> 
> .. || ref($disk) eq 'SCALAR'
> 
> as all else doe not makes sense, or?

on further thought, only the !ref($disks) makes any sense
(since we do not do any dereferencing)
and we should probably do something like

if (!ref) {
  disks = [ disks ];
} else if (ref != array) {
   die 'disks is not a string or arrayref';
}

> 
>> +	    $disks = [$disks];
> 
> nit: we normally use spaces between: [ $disks ] makes it easier to
> read.
> 
>> +	}
>> +
>> +	for my $disk (@$disks) {
>> +	    # we get cciss/c0d0 but need cciss!c0d0
>> +	    if (defined($disk) && $disk =~ m|^cciss/|) {
>> +		$disk =~ s|cciss/|cciss!|;
>> +	    }
> 
> just drop the if completely? should be $disk always defined and it
> makes no sense to do first a match and if it matches do the replace
> on the exact same match? just always do
> $disk =~ s|cciss/|cciss!|;
> 
> further, this whole for block above can be simply replaced by:
> 
> map { s|cciss/|cciss!| } @$disks;
> 
> does all you want, but in readable and short :) (I know you just moved this
> from the original, but as it's always worth to do some simple code-housekeeping
> as one touches things and you also wrote the original, this would be great to
> adapt)

yes, this is much more readable, thanks for the suggestion :)

> 
>> +	}
>> +
>> +	$path = "(?:" . join('|', @$disks) . ")";
>>       }
>>   
>> -    dir_glob_foreach('/sys/block', '.*', sub {
>> +    dir_glob_foreach('/sys/block', $path, sub {
>>   	my ($dev) = @_;
>> -	return if defined($disk) && $disk ne $dev;
>>   	# whitelisting following devices
>>   	# hdX: ide block device
>>   	# sdX: sd block device
>>
>