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

[Dominic Jäger]

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].

> > +    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)
?
> 
> or, if you want to make it shorter with something like:
> 
> my $dev;
> my $dev_sizes = { eval { map { $dev = $_ // ''; $_ => get_blockdev_size($_) } @$devices } };
> 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.

[0] https://pve.proxmox.com/pipermail/pve-devel/2020-March/042078.html