[pve-devel] [PATCH v3 storage 2/3] Incorporate wipe_disks from PVE::Ceph::Tools

[Thomas Lamprecht]

On 4/15/20 2:41 PM, Dominic Jäger wrote:
> Thank you for taking a look!
> Most makes sense to me and is already changed.
> 
> On Wed, Apr 15, 2020 at 11:28:34AM +0200, Fabian Grünbichler wrote:
>>
>>> +	if ($max_amount !~ /^[0-9]+$/);
>>
>> \d instead of [0-9]
> 
> This was \d previously. However, it was recommended to me to change it to [0-9]
> in v2 because of Unicode difficulties IIRC [0].
> 

albeit I'm not to sure if the unicode stuff is really true, I tried:
perl -we '"Ⅲ" =~ /\d/ or die "no match"'

where "Ⅲ" is the roman numeral three (U+2162) and that does not matches,
but it could be wrong encoding or whatever...


>>> +    foreach my $dev (@$devices) {
>>> +	my $device_size = get_blockdev_size($dev);
>>
>> add an eval here, and set $dev_sizes->{$dev} directly
>>
>>> +	die "Not wiping anything because could not get size of $dev." if !defined($device_size);
>>
>> and add ' - $@' to the message here. but note - you probably want to 
>> check $dev for definedness at the start of the loop, else this triggers 
>> a warning for printing undef ;)
> 
> "$dev for definedness at the start of the loop" like
> foreach my $dev (@$devices) {
>     if !defined($dev)
> ?

we often do:

next if !defined($dev);

at least if we expect that $dev can be really undef here..

>>
>> or, if you want to make it shorter with something like:
>>
>> my $dev;
>> my $dev_sizes = { eval { map { $dev = $_ // ''; $_ => get_blockdev_size($_) } @$devices } };

no use setting $dev if only $_ is used afterwards ;)

>> die "... $dev ... - $@\n" if $@;
>>
>> (could be made shorter if we don't care about including $dev in the 
>> error message, or make get_blockdev_size include it).
> 
> I'd have fun writing that but I can't see how the amount of reduced lines
> justifies the IMO increased complexity yet.

for this I'd just go for the `for my $..` loop, IMO the map isn't worth the
hassle here.