[pve-devel] applied: [PATCH guest-common v4 1/1] add check/exec_hookscript to GuestHelpers

[Thomas Lamprecht]

On 1/31/19 2:33 PM, Dominik Csapak wrote:
> check_hookscript will be used for the container/vm api to check if the
> hookscript volume id is correct
> 
> exec_hookscript can be called to execute a hookscript
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> changes from v3:
> * merged 2 commits
> * use check in exec
> * rename check
> * return path in check
>  PVE/GuestHelpers.pm | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/PVE/GuestHelpers.pm b/PVE/GuestHelpers.pm
> index c326812..892b6db 100644
> --- a/PVE/GuestHelpers.pm
> +++ b/PVE/GuestHelpers.pm
> @@ -4,6 +4,7 @@ use strict;
>  use warnings;
>  
>  use PVE::Tools;
> +use PVE::Storage;
>  
>  # We use a separate lock to block migration while a replication job
>  # is running.
> @@ -23,4 +24,50 @@ sub guest_migration_lock {
>      return $res;
>  }
>  
> +sub check_hookscript {
> +    my ($volid, $storecfg) = @_;
> +
> +    $storecfg = PVE::Storage::config() if !defined($storecfg);
> +    my ($path, undef, $type) = PVE::Storage::path($storecfg, $volid);
> +
> +    die "'$volid' is not in the snippets directory\n"
> +	if $type ne 'snippets';
> +
> +    die "script '$volid' does not exists\n"
> +	if ! -f $path;
> +
> +    die "script '$volid' is not executable\n"
> +	if ! -x $path;
> +
> +    return $path;
> +}
> +
> +sub exec_hookscript {
> +    my ($conf, $vmid, $phase, $stop_on_error) = @_;
> +
> +    return if !$conf->{hookscript};
> +    my $hookscript = eval { check_hookscript($conf->{hookscript}) };
> +    if (my $err = $@) {
> +	if ($stop_on_error) {
> +	    die $err;
> +	} else {
> +	    warn $err;
> +	    return;
> +	}
> +    }
> +
> +    eval {
> +	PVE::Tools::run_command([$hookscript, $vmid, $phase]);
> +    };
> +
> +    if (my $err = $@) {
> +	my $errmsg = "hookscript error for $vmid on $phase: $err\n";
> +	if ($stop_on_error) {
> +	    die $errmsg;
> +	} else {
> +	    warn $errmsg;
> +	}
> +    }
> +}

applied, but added a followup for the exception handling, if you use "eval + if $@"
as much as above you may want to reconsider to put all in a single big eval and
handle its exceptions in one place, can reduce code duplication and increase
readability. Thanks!

> +
>  1;
>