From 9aff3f3d8ea469249a88e44d7a5b87442610e534 Mon Sep 17 00:00:00 2001 From: Thomas Lamprecht Date: Fri, 23 Sep 2022 11:54:41 +0200 Subject: [PATCH] disk manage: module wide code/style cleanup 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 --- PVE/Diskmanage.pm | 213 ++++++++++++++++------------------------------ 1 file changed, 71 insertions(+), 142 deletions(-) diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm index d384694..f69bfd9 100644 --- a/PVE/Diskmanage.pm +++ b/PVE/Diskmanage.pm @@ -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 { -- 2.39.5