[pve-devel] [PATCH storage 3/5] Diskmanage: add append_partition sub

[Dominik Csapak]

On 6/4/19 10:25 AM, Thomas Lamprecht wrote:
> On 6/4/19 9:21 AM, Dominik Csapak wrote:
>> we will use this for adding a partition to a disk when using a device
>> for ceph osd db/wal which already has partitions on it
>>
>> first we search for the highest partition number, then add the partition
>> and search for the resulting device (we cannot assume to simply
>> append the number, e.g. from /dev/nvme0n1 we get /dev/nvme0n1pX)
>>
>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>> ---
>>   PVE/Diskmanage.pm | 36 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>
>> diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
>> index daad6df..645d67b 100644
>> --- a/PVE/Diskmanage.pm
>> +++ b/PVE/Diskmanage.pm
>> @@ -683,4 +683,40 @@ sub assert_disk_unused {
>>       return undef;
>>   }
>>   
>> +sub append_partition {
>> +    my ($dev, $size) = @_;
>> +
>> +    my $devname;
>> +    if ($dev =~ m|^/dev/(.*)$|) {
>> +	$devname = $1;
>> +    }
> 
> else ?? $devname can be undef and is used in string and regex below..
> either fallback or die?

oops, yes this should be handled...

> 
>> +
>> +    my $newpartid = 1;
>> +    dir_glob_foreach("/sys/block/$devname", qr/\Q$devname\E.+/, sub {
>> +	my ($part) = @_;
>> +
>> +	my ($partid) = $part =~ m/(\d+)$/;
>> +	$partid = int($partid);
> 
> could be undef, if no parition is present, not sure if you then can hit
> this code path at all, but maybe still worth dealing with that..
> And yes, I know that int(undef) == 0, but it produces also a warning
> and is a bit to subtle for my taste..

mhmm, right, better would be to include the (\d+) in the glob regex 
directly, so that we have it already extracted as parameter
e.g. qr/\Q$devname\E.*?(\d+)/

?

> 
>> +
>> +	if ($partid >= $newpartid) {
>> +	    $newpartid = $partid + 1;
>> +	}
>> +    });
>> +
>> +    $size = PVE::Tools::convert_size($size, 'b' => 'mb');
>> +
>> +    run_command([ $SGDISK, '-n', "$newpartid:0:+${size}M", $dev ],
>> +		errmsg => "error creating partition '$newpartid' on '$dev'");
>> +
>> +    my $partition;
>> +
>> +    dir_glob_foreach("/sys/block/$devname", qr/\Q$devname\E.*$newpartid/, sub {
>> +	my ($part) = @_;
>> +
>> +	$partition = "/dev/$part";
> 
> this is a, in a certain way, elegant solution, maybe adding a small
> comment could be good, something like:
> # new part name is dependent of underlying device type, "autocomplete" it

sure will do

> 
>> +    });
>> +
>> +    return $partition;
>> +}
>> +
>>   1;
>>
>