[pve-devel] [PATCH qemu-server 2/8] add Agent helper package

[Thomas Lamprecht]

Looks OK, few nits inline

On 6/7/18 1:16 PM, Dominik Csapak wrote:
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  PVE/API2/Qemu/Agent.pm  |  5 ++---
>  PVE/QemuServer/Agent.pm | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  PVE/QemuServer/Makefile |  1 +
>  3 files changed, 51 insertions(+), 3 deletions(-)
>  create mode 100644 PVE/QemuServer/Agent.pm
> 
> diff --git a/PVE/API2/Qemu/Agent.pm b/PVE/API2/Qemu/Agent.pm
> index 5141b4e..b965faf 100644
> --- a/PVE/API2/Qemu/Agent.pm
> +++ b/PVE/API2/Qemu/Agent.pm
> @@ -6,6 +6,7 @@ use warnings;
>  use PVE::RESTHandler;
>  use PVE::JSONSchema qw(get_standard_option);
>  use PVE::QemuServer;
> +use PVE::QemuServer::Agent qw(check_agent_available);
>  
>  use base qw(PVE::RESTHandler);
>  
> @@ -172,9 +173,7 @@ sub register_command {
>  
>  	    my $conf = PVE::QemuConfig->load_config ($vmid); # check if VM exists
>  
> -	    die "No Qemu Guest Agent\n" if !defined($conf->{agent});
> -	    die "VM $vmid is not running\n" if !PVE::QemuServer::check_running($vmid);
> -	    die "Qemu Guest Agent is not running\n" if !PVE::QemuServer::qga_check_running($vmid, 1);
> +	    check_agent_available($vmid, $conf);


PVE/QemuConfig.pm around line 164 and PVE/VZDump/QemuServer.pm around line 410
could maybe have similar replacements, even if they're not that straightforward
than this.

>  
>  	    my $cmd = $param->{command} // $command;
>  	    my $res = PVE::QemuServer::vm_mon_cmd($vmid, "guest-$cmd");
> diff --git a/PVE/QemuServer/Agent.pm b/PVE/QemuServer/Agent.pm
> new file mode 100644
> index 0000000..4dd3bde
> --- /dev/null
> +++ b/PVE/QemuServer/Agent.pm
> @@ -0,0 +1,48 @@
> +package PVE::QemuServer::Agent;
> +
> +use strict;
> +use warnings;
> +use PVE::QemuServer;
> +use base 'Exporter';
> +
> +our @EXPORT_OK = qw(
> +raise_agent_error
> +check_agent_available
> +);
> +
> +sub raise_agent_error {
> +    my ($result, $fallback_error, $noerr) = @_;
> +
> +    my $error = '';
> +    if (ref($result) eq 'HASH' && $result->{error} && $result->{error}->{desc}) {
> +	$error = "Agent Error: $result->{error}->{desc}\n";
> +    } elsif (!defined($result)) {
> +	$error = "Agent Error: $fallback_error\n";
> +    }
> +
> +    if ($noerr && $error) {
> +	warn $error;
> +	return undef;
> +    }
> +
> +    die $error if $error;

I'd rather have something like:

if ($error) {
    die $error if !$noerr;

    warn $error;
    return undef;
}

less conditions to read, follows more our general style and
keeps error handling in a single block.

> +
> +    return 1;
> +}
> +
> +sub check_agent_available {

maybe a shorter qga_available or just agent_available?

> +    my ($vmid, $conf, $noerr) = @_;
> +
> +    eval {
> +	die "No Qemu Guest Agent\n" if !defined($conf->{agent});
> +	die "VM $vmid is not running\n" if !PVE::QemuServer::check_running($vmid);
> +	die "Qemu Guest Agent is not running\n" if !PVE::QemuServer::qga_check_running($vmid, 1);
> +    };
> +
> +    return undef if $noerr && $@;
> +    die $@ if $@;

somewhat same here:

if (my $err = $@) {
    die $err if !$noerr;
    return undef;    
}

> +
> +    return 1;
> +}
> +
> +1;
> diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile
> index 0d78324..afc39a3 100644
> --- a/PVE/QemuServer/Makefile
> +++ b/PVE/QemuServer/Makefile
> @@ -4,6 +4,7 @@ SOURCES=PCI.pm		\
>  	ImportDisk.pm	\
>  	OVF.pm		\
>  	Cloudinit.pm	\
> +	Agent.pm	\
>  
>  .PHONY: install
>  install: ${SOURCES}
>