[pve-devel] [PATCH storage v2 1/2] add Diskmanage Utilities

[Dominik Csapak]

>> +
>> +    die "no such block device '$path'\n"
>> +	if ! -b $path;
>
> "Not a block device" might make more sense, since abs_path() says the
> path at least exists.
>

sounds right

>> +
>> +    die "not a valid disk" if $disk !~ m|^/dev/| ||  ! -b $disk;
>
> It might be worth factorizing out this check considering it's done below
> again with differently worded error messages. Would improve consistency.
>

>> +
>> +    die "not a device" if $disk !~ m|^/dev/|;
>
> Should this contain a -b check as well? If so, it can use the
> factorized version as suggested above. Note the wording used in this
> error.
>

>> +    return "NOT A DEVICE" if $disk !~ m|^/dev/| || ! -b $disk;
>
> This is the same check as the second one I marked and uses a different
> and capitalized error message. So again, an assert_blockdev($) function
> with a sane/consistent error message would be useful here.
>
>

yeah, you're right. this should be factored out

>> +
>> +    my $fd = IO::File->new("/proc/mounts", "r") ||
>> +	die "unable to open /proc/mounts - $!\n";
>
> Looks like left-over code. You use parse_proc_mounts() below and this
> $fd is not used at all anymore.
>

yes, this is just left-over code


i'll wait for further comments, and prepare the next version

if nobody has a comment on the general structure or api calls, i'll
prepare the gui component for the disk management in the next days