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

[Thomas Lamprecht]

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

> +    if (defined($disks)) {
> +	if (!ref($disks) || ref($disks) ne 'ARRAY') {

.. || ref($disk) eq 'SCALAR'

as all else doe not makes sense, or?

> +	    $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)

> +	}
> +
> +	$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
>