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

[Dietmar Maurer]

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 {