[pve-devel] [PATCH qemu-server 4/7] make an api call for each guest agent command

[Thomas Lamprecht]

On 2/13/18 4:47 PM, Dominik Csapak wrote:
> with a 'register_command' sub, which generates an api call
> we call it for each command in the list, and one time for
> the old general {vmid}/agent endpoint (for compatibility)
> 
> permissions/methods are the same as previously, but can
> be overriden with an entry in $ga_cmd_properties
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  PVE/API2/Qemu/Agent.pm | 99 ++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 71 insertions(+), 28 deletions(-)
> 
> diff --git a/PVE/API2/Qemu/Agent.pm b/PVE/API2/Qemu/Agent.pm
> index 437d3f6..9d87b43 100644
> --- a/PVE/API2/Qemu/Agent.pm
> +++ b/PVE/API2/Qemu/Agent.pm
> @@ -9,6 +9,8 @@ use PVE::QemuServer;
>  
>  use base qw(PVE::RESTHandler);
>  
> +# list of commands
> +# will generate one api endpoint per command
>  my $guest_agent_commands = [
>      'ping',
>      'get-time',
> @@ -28,49 +30,90 @@ my $guest_agent_commands = [
>      'shutdown',
>      ];
>  
> -__PACKAGE__->register_method({
> -    name => 'agent',
> -    path => '',
> -    method => 'POST',
> -    protected => 1,
> -    proxyto => 'node',
> -    description => "Execute Qemu Guest Agent commands.",
> -    permissions => {
> -	check => ['perm', '/vms/{vmid}', [ 'VM.Monitor' ]],
> -    },
> -    parameters => {
> +# properties for each command, optional
> +# can has

What's with the early broken line? If we'd keep the semantics here I'd swap
the whole with:

# optionally set perms (default: VM.Monitor) or method (default: POST) property

To make a short concise single line comment.

> +# 'method': e.g. GET/POST
> +# 'perms': either a string like 'VM.Montior' or an array of such strings
> +#	   or a permission object

s/Montior/Monitor/

> +my $ga_cmd_properties =  {};
> +
> +sub register_command {
> +    my ($class, $command, $method, $perm) = @_;
> +
> +    $method //= 'POST';
> +    my $permission;
> +    if (!$perm) {
> +	$permission = { check => [ 'perm', '/vms/{vmid}', [ 'VM.Monitor' ]]};
> +    } elsif (ref($perm) eq 'SCALAR') {
> +	$permission = { check => [ 'perm', '/vms/{vmid}', [ $perm ]]};
> +    } elsif (ref($perm) eq 'ARRAY') {
> +	$permission = { check => [ 'perm', '/vms/{vmid}', $perm ]};
> +    } elsif (ref($perm) eq 'HASH') {
> +	$permission = $perm;
> +    } else {
> +	die 'invalid permissions given';
> +    }

That's a bit of code which currently does not get used once after
the whole series got applied?
You sure we need this all? If so either add the respective permission
changes together if some monitor commands would need them, or just add
a very simplified version which accepts either string (falling back to
VM.Monitor) or full hash for now?

Rest looks OK.

> +
> +    my $parameters = {
>  	additionalProperties => 0,
>  	properties => {
>  	    node => get_standard_option('pve-node'),
>  	    vmid => get_standard_option('pve-vmid', {
> -                   completion => \&PVE::QemuServer::complete_vmid_running }),
> +		    completion => \&PVE::QemuServer::complete_vmid_running }),
>  	    command => {
>  		type => 'string',
>  		description => "The QGA command.",
>  		enum => $guest_agent_commands,
>  	    },
>  	},
> -    },
> -    returns => {
> -	type => 'object',
> -	description => "Returns an object with a single `result` property. The type of that
> -property depends on the executed command.",
> -    },
> -    code => sub {
> -	my ($param) = @_;
> +    };
> +
> +    my $description = "Execute Qemu Guest Agent commands.";
> +    my $name = 'agent';
> +    $command //= '';
> +
> +    if ($command ne '') {
> +	$description = "Execute $command.";
> +	$name = $command;
> +	delete $parameters->{properties}->{command};
> +    }
> +
> +    __PACKAGE__->register_method({
> +	name => $name,
> +	path => $command,
> +	method => $method,
> +	protected => 1,
> +	proxyto => 'node',
> +	description => $description,
> +	permissions => $permission,
> +	parameters => $parameters,
> +	returns => {
> +	    type => 'object',
> +	    description => "Returns an object with a single `result` property.",
> +	},
> +	code => sub {
> +	    my ($param) = @_;
> +
> +	    my $vmid = $param->{vmid};
>  
> -	my $vmid = $param->{vmid};
> +	    my $conf = PVE::QemuConfig->load_config ($vmid); # check if VM exists
>  
> -	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 "No Qemu Guest Agent\n" if !defined($conf->{agent});
> -	die "VM $vmid is not running\n" if !PVE::QemuServer::check_running($vmid);
> +	    my $cmd = $param->{command} // $command;
> +	    my $res = PVE::QemuServer::vm_mon_cmd($vmid, "guest-$cmd");
>  
> -	my $cmd = $param->{command};
> +	    return { result => $res };
> +	}});
> +}
>  
> -	my $res = PVE::QemuServer::vm_mon_cmd($vmid, "guest-$cmd");
> +# old {vmid}/agent POST endpoint, here for compatibility
> +__PACKAGE__->register_command();
>  
> -	return { result => $res };
> -    }});
> +for my $cmd (@$guest_agent_commands) {
> +    my $props = $ga_cmd_properties->{$cmd} // {};
> +    __PACKAGE__->register_command($cmd, $props->{method}, $props->{perms});
> +}
>  
>  1;
>