]> git.proxmox.com Git - pve-storage.git/commitdiff
disk manage: module wide code/style cleanup
authorThomas Lamprecht <t.lamprecht@proxmox.com>
Fri, 23 Sep 2022 09:54:41 +0000 (11:54 +0200)
committerThomas Lamprecht <t.lamprecht@proxmox.com>
Fri, 23 Sep 2022 09:56:55 +0000 (11:56 +0200)
fixing some issues reported by perlcritic along the way.

cutting down 70 lines, often with even improving readability.
Tried to recheck and be conservative, so there shouldn't be any
regression, but it's still perl after all...

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
PVE/Diskmanage.pm

index d384694e0ae085d2e6510e9fe0f4eef6bd472d49..f69bfd917794a25813a7bb2e8161d88195149f49 100644 (file)
@@ -51,7 +51,7 @@ sub assert_blockdev {
     my ($dev, $noerr) = @_;
 
     if ($dev !~ m|^/dev/| || !(-b $dev)) {
-       return undef if $noerr;
+       return if $noerr;
        die "not a valid block device\n";
     }
 
@@ -93,8 +93,6 @@ sub get_smart_data {
     my $smartdata = {};
     my $type;
 
-    my $returncode = 0;
-
     if ($disk =~ m!^/dev/(nvme\d+n\d+)$!) {
        my $info = get_sysdir_info("/sys/block/$1");
        $disk = "/dev/".($info->{device}
@@ -105,8 +103,8 @@ sub get_smart_data {
     push @$cmd, '-A', '-f', 'brief' if !$healthonly;
     push @$cmd, $disk;
 
-    eval {
-       $returncode = run_command($cmd, noerr => 1, outfunc => sub{
+    my $returncode = eval {
+       run_command($cmd, noerr => 1, outfunc => sub {
            my ($line) = @_;
 
 # ATA SMART attributes, e.g.:
@@ -154,13 +152,11 @@ sub get_smart_data {
            } elsif ($line =~ m/SMART Disabled/) {
                $smartdata->{health} = "SMART Disabled";
            }
-       });
+       })
     };
     my $err = $@;
 
-    # bit 0 and 1 mark an severe smartctl error
-    # all others are for disk status, so ignore them
-    # see smartctl(8)
+    # bit 0 and 1 mark a fatal error, other bits are for disk status -> ignore (see man 8 smartctl)
     if ((defined($returncode) && ($returncode & 0b00000011)) || $err) {
        die "Error getting S.M.A.R.T. data: Exit code: $returncode\n";
     }
@@ -170,34 +166,28 @@ sub get_smart_data {
     return $smartdata;
 }
 
-sub get_lsblk_info() {
+sub get_lsblk_info {
     my $cmd = [$LSBLK, '--json', '-o', 'path,parttype,fstype'];
     my $output = "";
-    my $res = {};
-    eval {
-       run_command($cmd, outfunc => sub {
-           my ($line) = @_;
-           $output .= "$line\n";
-       });
-    };
+    eval { run_command($cmd, outfunc => sub { $output .= "$_[0]\n"; }) };
     warn "$@\n" if $@;
-    return $res if $output eq '';
+    return {} if $output eq '';
 
-    my $parsed = eval { decode_json($output) };
+    my $parsed = eval { decode_json($output) } // {};
     warn "$@\n" if $@;
     my $list = $parsed->{blockdevices} // [];
 
-    $res = { map {
-       $_->{path} => {
-           parttype => $_->{parttype},
-           fstype => $_->{fstype}
-       }
-    } @{$list} };
-
-    return $res;
+    return {
+       map {
+           $_->{path} => {
+               parttype => $_->{parttype},
+               fstype => $_->{fstype}
+           }
+       } @{$list}
+    };
 }
 
-my $get_devices_by_partuuid = sub {
+my sub get_devices_by_partuuid {
     my ($lsblk_info, $uuids, $res) = @_;
 
     $res = {} if !defined($res);
@@ -209,7 +199,7 @@ my $get_devices_by_partuuid = sub {
     }
 
     return $res;
-};
+}
 
 sub get_zfs_devices {
     my ($lsblk_info) = @_;
@@ -217,21 +207,17 @@ sub get_zfs_devices {
 
     return {} if !check_bin($ZPOOL);
 
-    # use zpool and parttype uuid,
-    # because log and cache do not have
-    # zfs type uuid
+    # use zpool and parttype uuid, because log and cache do not have zfs type uuid
     eval {
        run_command([$ZPOOL, 'list', '-HPLv'], outfunc => sub {
             my ($line) = @_;
-
             if ($line =~ m|^\t([^\t]+)\t|) {
                $res->{$1} = 1;
             }
        });
     };
 
-    # only warn here,
-    # because maybe zfs tools are not installed
+    # only warn here, because maybe zfs tools are not installed
     warn "$@\n" if $@;
 
     my $uuids = {
@@ -240,7 +226,7 @@ sub get_zfs_devices {
     };
 
 
-    $res = $get_devices_by_partuuid->($lsblk_info, $uuids, $res);
+    $res = get_devices_by_partuuid($lsblk_info, $uuids, $res);
 
     return $res;
 }
@@ -258,15 +244,14 @@ sub get_lvm_devices {
        });
     };
 
-    # if something goes wrong, we do not want
-    # to give up, but indicate an error has occurred
+    # if something goes wrong, we do not want to give up, but indicate an error has occurred
     warn "$@\n" if $@;
 
     my $uuids = {
        "e6d6d379-f507-44c2-a23c-238f2a3df928" => 1,
     };
 
-    $res = $get_devices_by_partuuid->($lsblk_info, $uuids, $res);
+    $res = get_devices_by_partuuid($lsblk_info, $uuids, $res);
 
     return $res;
 }
@@ -282,7 +267,7 @@ sub get_ceph_journals {
        'cafecafe-9b03-4f30-b4c6-b4b80ceff106' => 4, # block
     };
 
-    $res = $get_devices_by_partuuid->($lsblk_info, $uuids, $res);
+    $res = get_devices_by_partuuid($lsblk_info, $uuids, $res);
 
     return $res;
 }
@@ -333,46 +318,31 @@ sub get_udev_info {
        });
     };
     warn $@ if $@;
-    return undef if !$info;
+    return if !$info;
 
-    return undef if $info !~ m/^E: DEVTYPE=(disk|partition)$/m;
-    return undef if $info =~ m/^E: ID_CDROM/m;
+    return if $info !~ m/^E: DEVTYPE=(disk|partition)$/m;
+    return if $info =~ m/^E: ID_CDROM/m;
 
-    # we use this, because some disks are not simply in /dev
-    # e.g. /dev/cciss/c0d0
+    # we use this, because some disks are not simply in /dev e.g. /dev/cciss/c0d0
     if ($info =~ m/^E: DEVNAME=(\S+)$/m) {
        $data->{devpath} = $1;
     }
     return if !defined($data->{devpath});
 
     $data->{serial} = 'unknown';
-    if ($info =~ m/^E: ID_SERIAL_SHORT=(\S+)$/m) {
-       $data->{serial} = $1;
-    }
+    $data->{serial} = $1 if $info =~ m/^E: ID_SERIAL_SHORT=(\S+)$/m;
 
-    $data->{gpt} = 0;
-    if ($info =~ m/^E: ID_PART_TABLE_TYPE=gpt$/m) {
-       $data->{gpt} = 1;
-    }
+    $data->{gpt} = $info =~ m/^E: ID_PART_TABLE_TYPE=gpt$/m ? 1 : 0;
 
-    # detect SSD
     $data->{rpm} = -1;
-    if ($info =~ m/^E: ID_ATA_ROTATION_RATE_RPM=(\d+)$/m) {
-       $data->{rpm} = $1;
-    }
+    $data->{rpm} = $1 if $info =~ m/^E: ID_ATA_ROTATION_RATE_RPM=(\d+)$/m; # detects SSD implicit
 
-    if ($info =~ m/^E: ID_BUS=usb$/m) {
-       $data->{usb} = 1;
-    }
+    $data->{usb} = 1 if $info =~ m/^E: ID_BUS=usb$/m;
 
-    if ($info =~ m/^E: ID_MODEL=(.+)$/m) {
-       $data->{model} = $1;
-    }
+    $data->{model} = $1 if $info =~ m/^E: ID_MODEL=(.+)$/m;
 
     $data->{wwn} = 'unknown';
-    if ($info =~ m/^E: ID_WWN=(.*)$/m) {
-       $data->{wwn} = $1;
-    }
+    $data->{wwn} = $1 if $info =~ m/^E: ID_WWN=(.*)$/m;
 
     if ($info =~ m/^E: DEVLINKS=(.+)$/m) {
        my @devlinks = grep(m#^/dev/disk/by-id/(ata|scsi|nvme(?!-eui))#, split (/ /, $1));
@@ -388,15 +358,14 @@ sub get_sysdir_size {
     my $size = file_read_firstline("$sysdir/size");
     return if !$size;
 
-    # linux always considers sectors to be 512 bytes,
-    # independently of real block size
+    # linux always considers sectors to be 512 bytes, independently of real block size
     return $size * 512;
 }
 
 sub get_sysdir_info {
     my ($sysdir) = @_;
 
-    return undef if ! -d "$sysdir/device";
+    return if ! -d "$sysdir/device";
 
     my $data = {};
 
@@ -409,8 +378,7 @@ sub get_sysdir_info {
     $data->{model} = file_read_firstline("$sysdir/device/model") || 'unknown';
 
     if (defined(my $device = readlink("$sysdir/device"))) {
-       # strip directory and untaint:
-       ($data->{device}) = $device =~ m!([^/]+)$!;
+       ($data->{device}) = $device =~ m!([^/]+)$!; # strip directory and untaint
     }
 
     return $data;
@@ -426,9 +394,8 @@ sub get_wear_leveling_info {
 
     my $wearout;
 
-    # Common register names that represent percentage values of potential
-    # failure indicators used in drivedb.h of smartmontool's. Order matters,
-    # as some drives may have multiple definitions
+    # Common register names that represent percentage values of potential failure indicators used
+    # in drivedb.h of smartmontool's. Order matters, as some drives may have multiple definitions
     my @wearoutregisters = (
        "Media_Wearout_Indicator",
        "SSD_Life_Left",
@@ -553,18 +520,17 @@ sub get_disks {
     dir_glob_foreach('/sys/block', $disk_regex, sub {
        my ($dev) = @_;
        # whitelisting following devices
-       # hdX: ide block device
-       # sdX: sd block device
-       # vdX: virtual block device
-       # xvdX: xen virtual block device
-       # nvmeXnY: nvme devices
-       # cciss!cXnY: cciss devices
+       # - hdX         ide block device
+       # - sdX         scsi/sata block device
+       # - vdX         virtIO block device
+       # - xvdX:       xen virtual block device
+       # - nvmeXnY:    nvme devices
+       # - cciss!cXnY  cciss devices
        return if $dev !~ m/^(h|s|x?v)d[a-z]+$/ &&
                  $dev !~ m/^nvme\d+n\d+$/ &&
                  $dev !~ m/^cciss\!c\d+d\d+$/;
 
-       my $data = get_udev_info("/sys/block/$dev");
-       return if !defined($data);
+       my $data = get_udev_info("/sys/block/$dev") // return;
        my $devpath = $data->{devpath};
 
        my $sysdir = "/sys/block/$dev";
@@ -590,26 +556,21 @@ sub get_disks {
            }
        }
 
-       my $health = 'UNKNOWN';
-       my $wearout = 'N/A';
-
+       my ($health, $wearout) = ('UNKNOWN', 'N/A');
        if (!$nosmart) {
            eval {
                my $smartdata = get_smart_data($devpath, !is_ssdlike($type));
                $health = $smartdata->{health} if $smartdata->{health};
 
-               if (is_ssdlike($type)) {
-                   # if we have an ssd we try to get the wearout indicator
-                   my $wearval = get_wear_leveling_info($smartdata);
-                   $wearout = $wearval if defined($wearval);
+               if (is_ssdlike($type)) { # if we have an ssd we try to get the wearout indicator
+                   my $wear_level = get_wear_leveling_info($smartdata);
+                   $wearout = $wear_level if defined($wear_level);
                }
            };
        }
 
-       # we replaced cciss/ with cciss! above
-       # but in the result we need cciss/ again
-       # because the caller might want to check the
-       # result again with the original parameter
+       # we replaced cciss/ with cciss! above, but in the result we need cciss/ again because the
+       # caller might want to check the result again with the original parameter
        if ($dev =~ m|^cciss!|) {
            $dev =~ s|^cciss!|cciss/|;
        }
@@ -632,19 +593,11 @@ sub get_disks {
        my $by_id_link = $data->{by_id_link};
        $disklist->{$dev}->{by_id_link} = $by_id_link if defined($by_id_link);
 
-       my $osdid = -1;
-       my $bluestore = 0;
-       my $osdencrypted = 0;
-
-       my $journal_count = 0;
-       my $db_count = 0;
-       my $wal_count = 0;
+       my ($osdid, $bluestore, $osdencrypted) = (-1, 0, 0);
+       my ($journal_count, $db_count, $wal_count) = (0, 0, 0);
 
        my $partpath = $devpath;
-
-       # remove part after last / to
-       # get the base path for the partitions
-       # e.g. from /dev/cciss/c0d0 get /dev/cciss
+       # remove trailing part to get the partition base path, e.g. /dev/cciss/c0d0 -> /dev/cciss
        $partpath =~ s/\/[^\/]+$//;
 
        my $determine_usage = sub {
@@ -655,18 +608,13 @@ sub get_disks {
 
            my $info = $lsblk_info->{$devpath} // {};
 
-           my $parttype = $info->{parttype};
-           if (defined($parttype)) {
-               return 'BIOS boot'
-                   if $parttype eq '21686148-6449-6e6f-744e-656564454649';
-               return 'EFI'
-                   if $parttype eq 'c12a7328-f81f-11d2-ba4b-00a0c93ec93b';
-               return 'ZFS reserved'
-                   if $parttype eq '6a945a3b-1dd2-11b2-99a6-080020736631';
+           if (defined(my $parttype = $info->{parttype})) {
+               return 'BIOS boot'if $parttype eq '21686148-6449-6e6f-744e-656564454649';
+               return 'EFI' if $parttype eq 'c12a7328-f81f-11d2-ba4b-00a0c93ec93b';
+               return 'ZFS reserved' if $parttype eq '6a945a3b-1dd2-11b2-99a6-080020736631';
            }
 
-           my $fstype = $info->{fstype};
-           return "${fstype}" if defined($fstype);
+           return "$info->{fstype}" if defined($info->{fstype});
            return 'mounted' if $mounted->{$devpath};
 
            return if !$is_partition;
@@ -691,13 +639,11 @@ sub get_disks {
            }
 
            my $result = { %{$ceph_volume} };
-           $result->{journals} = delete $result->{journal}
-               if $result->{journal};
+           $result->{journals} = delete $result->{journal} if $result->{journal};
            return $result;
        };
 
        my $partitions = {};
-
        dir_glob_foreach("$sysdir", "$dev.+", sub {
            my ($part) = @_;
 
@@ -709,18 +655,14 @@ sub get_disks {
            $partitions->{$part}->{mounted} = 1 if exists $mounted->{"$partpath/$part"};
            $partitions->{$part}->{gpt} = $data->{gpt};
            $partitions->{$part}->{type} = 'partition';
-           $partitions->{$part}->{size} =
-               get_sysdir_size("$sysdir/$part") // 0;
-           $partitions->{$part}->{used} =
-               $determine_usage->("$partpath/$part", "$sysdir/$part", 1);
+           $partitions->{$part}->{size} = get_sysdir_size("$sysdir/$part") // 0;
+           $partitions->{$part}->{used} = $determine_usage->("$partpath/$part", "$sysdir/$part", 1);
            $partitions->{$part}->{osdid} //= -1;
 
-           # Avoid counting twice (e.g. partition on which the LVM for the
-           # DB OSD resides is present in the $journalhash)
+           # avoid counting twice (e.g. partition with the LVM for the DB OSD is in $journalhash)
            return if $lvm_based_osd;
 
            # Legacy handling for non-LVM based OSDs
-
            if (my $mp = $mounted->{"$partpath/$part"}) {
                if ($mp =~ m|^/var/lib/ceph/osd/ceph-(\d+)$|) {
                    $osdid = $1;
@@ -768,14 +710,11 @@ sub get_disks {
        $disklist->{$dev}->{wal} = $wal_count if $wal_count;
 
        if ($include_partitions) {
-           foreach my $part (keys %{$partitions}) {
-               $disklist->{$part} = $partitions->{$part};
-           }
+           $disklist->{$_} = $partitions->{$_} for keys %{$partitions};
        }
     });
 
     return $disklist;
-
 }
 
 sub get_partnum {
@@ -790,19 +729,11 @@ sub get_partnum {
     my $minor = PVE::Tools::dev_t_minor($st->rdev);
     my $partnum_path = "/sys/dev/block/$major:$minor/";
 
-    my $partnum;
-
-    $partnum = file_read_firstline("${partnum_path}partition");
-
+    my $partnum = file_read_firstline("${partnum_path}partition");
     die "Partition does not exist\n" if !defined($partnum);
-
-    #untaint and ensure it is a int
-    if ($partnum =~ m/(\d+)/) {
-       $partnum = $1;
-       die "Partition number $partnum is invalid\n" if $partnum > 128;
-    } else {
-       die "Failed to get partition number\n";
-    }
+    die "Failed to get partition number\n" if $partnum !~ m/(\d+)/; # untaint
+    $partnum = $1;
+    die "Partition number $partnum is invalid\n" if $partnum > 128;
 
     return $partnum;
 }
@@ -841,10 +772,8 @@ sub locked_disk_action {
 
 sub assert_disk_unused {
     my ($dev) = @_;
-
     die "device '$dev' is already in use\n" if disk_is_used($dev);
-
-    return undef;
+    return;
 }
 
 sub append_partition {