[pve-devel] [PATCH storage v2 1/2] add Diskmanage Utilities
d.csapak at proxmox.com
Mon Jun 6 14:01:27 CEST 2016
>> + 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.
>> + 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
>> + 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
More information about the pve-devel