[pve-devel] [PATCH] Storage: add methode

[Wolfgang Link]

>     +    } else {
>     +        die "can't snapshot";
    why 'can't snapshot'?!


If the storeid can not be parsed (undef)!

>     -    } elsif ($volid =~ m|^(/.+)$| && -e $volid) {
>     -        die "snapshot rollback device is not possible";
>     -    } else {
>     -        die "can't snapshot";

    Why do you remove those checks?

Because I checked them before in QemuServer.pm!
should I double check it?

>       -    # abort rollback if snapshot is not the latest
>     -    my $recentsnap = $class->zfs_get_latest_snapshot($scfg,
>     $volname);
>     -    if ($snap ne $recentsnap) {
>     -        die "cannot rollback, more recent snapshots exist\n";
>     -    }
>     -

    You simple remove that test (but do not call
    volume_rollback_is_possibe)?

I call it in QemuServer.pm.
should I double check it?

On 02/12/2015 06:57 AM, Dietmar Maurer wrote:
> On 02/11/2015 01:33 PM, Wolfgang Link wrote:
>> add methode volume_rollback_is_possibel and refactor
>> Improve error handling
>> If snapshot is not revetable catch it before vm will lock and shutdown.
>> This is the case if zfs has an younger snapshot.
> Please use a spell checker to fix spelling errors.
>
> possibel => possible
> revetable => ?
>
>>
>> Signed-off-by: Wolfgang Link <w.link at proxmox.com>
>> ---
>>   PVE/Storage.pm               |   19 +++++++++++++++----
>>   PVE/Storage/Plugin.pm        |    6 ++++++
>>   PVE/Storage/ZFSPlugin.pm     |    6 ------
>>   PVE/Storage/ZFSPoolPlugin.pm |   11 ++++++++---
>>   4 files changed, 29 insertions(+), 13 deletions(-)
>>
>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>> index 8711710..c4b9a68 100755
>> --- a/PVE/Storage.pm
>> +++ b/PVE/Storage.pm
>> @@ -144,6 +144,21 @@ sub volume_resize {
>>       }
>>   }
>>   +sub volume_rollback_is_possibel {
>> +    my ($cfg, $volid, $snap) = @_;
>> +
>> +    my ($storeid, $volname) = parse_volume_id($volid, 1);
>> +    if ($storeid) {
>> +        my $scfg = storage_config($cfg, $storeid);
>> +        my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>> +        return $plugin->volume_rollback_is_possibe($scfg, $storeid, 
>> $volname, $snap);
>
> volume_rollback_is_possibe => volume_rollback_is_possible
>
>
>> +     } elsif ($volid =~ m|^(/.+)$| && -e $volid) {
>> +        die "snapshot rollback device is not possible";
>
> We can improve those error messages, for example:
>
> die "rollback device '$volid' is not possible\n";
>
>
>> +    } else {
>> +        die "can't snapshot";
> why 'can't snapshot'?!
>
>> +    }
>> +}
>> +
>>   sub volume_snapshot {
>>       my ($cfg, $volid, $snap, $running) = @_;
>>   @@ -167,10 +182,6 @@ sub volume_snapshot_rollback {
>>           my $scfg = storage_config($cfg, $storeid);
>>           my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>>           return $plugin->volume_snapshot_rollback($scfg, $storeid, 
>> $volname, $snap);
>> -    } elsif ($volid =~ m|^(/.+)$| && -e $volid) {
>> -        die "snapshot rollback device is not possible";
>> -    } else {
>> -        die "can't snapshot";
>
> Why do you remove those checks?
>>       }
>>   }
>>   diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>> index 15c23d4..4549f9f 100644
>> --- a/PVE/Storage/Plugin.pm
>> +++ b/PVE/Storage/Plugin.pm
>> @@ -656,6 +656,12 @@ sub volume_snapshot {
>>       return undef;
>>   }
>>   +sub volume_rollback_is_possibe {
>> +    my ($class, $scfg, $storeid, $volname, $snap) = @_;
>> +
>> +    return 1;
>> +}
>> +
>>   sub volume_snapshot_rollback {
>>       my ($class, $scfg, $storeid, $volname, $snap) = @_;
>>   diff --git a/PVE/Storage/ZFSPlugin.pm b/PVE/Storage/ZFSPlugin.pm
>> index 77394b9..f020985 100644
>> --- a/PVE/Storage/ZFSPlugin.pm
>> +++ b/PVE/Storage/ZFSPlugin.pm
>> @@ -310,12 +310,6 @@ sub volume_resize {
>>   sub volume_snapshot_rollback {
>>       my ($class, $scfg, $storeid, $volname, $snap) = @_;
>>   -    # abort rollback if snapshot is not the latest
>> -    my $recentsnap = $class->zfs_get_latest_snapshot($scfg, $volname);
>> -    if ($snap ne $recentsnap) {
>> -        die "cannot rollback, more recent snapshots exist\n";
>> -    }
>> -
>
> You simple remove that test (but do not call volume_rollback_is_possibe)?
>>       $class->zfs_delete_lu($scfg, $volname);
>>         $class->zfs_request($class, $scfg, undef, 'rollback', 
>> "$scfg->{pool}/$volname\@$snap");
>> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
>> index 754f29f..0d38012 100644
>> --- a/PVE/Storage/ZFSPoolPlugin.pm
>> +++ b/PVE/Storage/ZFSPoolPlugin.pm
>> @@ -411,13 +411,18 @@ sub volume_snapshot_delete {
>>   sub volume_snapshot_rollback {
>>       my ($class, $scfg, $storeid, $volname, $snap) = @_;
>>   -    # abort rollback if snapshot is not the latest
>> +    zfs_request($class, $scfg, undef, 'rollback', 
>> "$scfg->{pool}/$volname\@$snap");
>> +}
>> +
>> +sub volume_rollback_is_possibe {
>> +    my ($class, $scfg, $storeid, $volname, $snap) = @_;
>> +
>>       my $recentsnap = $class->zfs_get_latest_snapshot($scfg, $volname);
>>       if ($snap ne $recentsnap) {
>> -        die "cannot rollback, more recent snapshots exist\n";
>> +    die "cannot rollback, more recent snapshots exist\n";
>>       }
>>   -    zfs_request($class, $scfg, undef, 'rollback', 
>> "$scfg->{pool}/$volname\@$snap");
>> +    return 1;
>>   }
>>     sub activate_storage {
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.proxmox.com/pipermail/pve-devel/attachments/20150212/033ee9cd/attachment.htm>