[pve-devel] [PATCH qemu-server 5/8] implement agent exec api call

[Thomas Lamprecht]

On 6/7/18 1:16 PM, Dominik Csapak wrote:
> this imitates the qemu-guest-agent interface
> with an 'exec' api call which returns a pid
> and an 'exec-status' api call which takes a pid
> 
> the command for the exec call is given as an 'alist'
> which means that when using we have to give the 'command'
> parameter multiple times e.g.
> 
> pvesh create <...>/exec --command ls --command '-lha' --command '/home/user'
> 
> so that we avoid having to deal with shell escaping etc.
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  PVE/API2/Qemu/Agent.pm  | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>  PVE/QemuServer/Agent.pm | 44 +++++++++++++++++++++++++++
>  2 files changed, 125 insertions(+)
> 
> diff --git a/PVE/API2/Qemu/Agent.pm b/PVE/API2/Qemu/Agent.pm
> index be6ef15..ebc5503 100644
> --- a/PVE/API2/Qemu/Agent.pm
> +++ b/PVE/API2/Qemu/Agent.pm
> @@ -112,6 +112,8 @@ __PACKAGE__->register_method({
>  
>  	my $cmds = [keys %$guest_agent_commands];
>  	push @$cmds, qw(
> +	    exec
> +	    exec-status
>  	    set-user-password
>  	);
>  
> @@ -254,4 +256,83 @@ __PACKAGE__->register_method({
>  	return { result => $res };
>      }});
>  
> +__PACKAGE__->register_method({
> +    name => 'exec',
> +    path => 'exec',
> +    method => 'POST',
> +    protected => 1,
> +    proxyto => 'node',
> +    description => "Executes the given command in the vm via the guest agent.",
> +    permissions => { check => [ 'perm', '/vms/{vmid}', [ 'VM.Monitor' ]]},
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    vmid => get_standard_option('pve-vmid', {
> +		    completion => \&PVE::QemuServer::complete_vmid_running }),
> +	    command => {
> +		type => 'string',
> +		format => 'string-alist',
> +		description => 'The command as a list of program + arguments',

"... consisting of the filename to execute, followed by its arguments"

> +	    }

How about input data for the command?
guest-exec supports a input-data parameter, isn't that useful or did you
just want to keep it basic for now?

> +	},
> +    },
> +    returns => {
> +	type => 'object',
> +	description => "Returns an object with a single `result` property.",

Hmm, better description and even more strict return properties?
QEMU's QAPI schema says:

> @guest-exec:
> Execute a command in the guest
> [...]
> Returns: PID on success.

So why not just say (in words and schema language) that we return:
{ pid => int }
?

> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $vmid = $param->{vmid};
> +
> +	my $conf = PVE::QemuConfig->load_config ($vmid); # check if VM exists
> +
> +	check_agent_available($vmid, $conf);
> +
> +	my $cmd = [PVE::Tools::split_list($param->{command})];
> +	my $res = PVE::QemuServer::Agent::qemu_exec($vmid, $cmd);
> +	return { result => $res };
> +    }});
> +
> +__PACKAGE__->register_method({
> +    name => 'exec-status',
> +    path => 'exec-status',
> +    method => 'GET',
> +    protected => 1,
> +    proxyto => 'node',
> +    description => "Gets the status of the given pid",

I'd maybe note that only PIDs from proccesses started by qga are
valid here, not arbitrary PIDs - like seeing if PID 1 sill runs.

> +    permissions => { check => [ 'perm', '/vms/{vmid}', [ 'VM.Monitor' ]]},
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => get_standard_option('pve-node'),
> +	    vmid => get_standard_option('pve-vmid', {
> +		    completion => \&PVE::QemuServer::complete_vmid_running }),
> +	    pid => {
> +		type => 'integer',
> +		description => 'The PID to query'
> +	    },
> +	},
> +    },
> +    returns => {
> +	type => 'object',
> +	description => "Returns an object with a single `result` property.",

same as above

> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $vmid = $param->{vmid};
> +
> +	my $conf = PVE::QemuConfig->load_config ($vmid); # check if VM exists
> +
> +	check_agent_available($vmid, $conf);
> +
> +	my $pid = int($param->{pid});
> +
> +	my $res = PVE::QemuServer::Agent::qemu_exec_status($vmid, $pid);
> +
> +	return { result => $res };
> +    }});
> +
>  1;
> diff --git a/PVE/QemuServer/Agent.pm b/PVE/QemuServer/Agent.pm
> index 4dd3bde..6e3fce9 100644
> --- a/PVE/QemuServer/Agent.pm
> +++ b/PVE/QemuServer/Agent.pm
> @@ -3,6 +3,7 @@ package PVE::QemuServer::Agent;
>  use strict;
>  use warnings;
>  use PVE::QemuServer;
> +use MIME::Base64 qw(decode_base64);
>  use base 'Exporter';
>  
>  our @EXPORT_OK = qw(
> @@ -45,4 +46,47 @@ sub check_agent_available {
>      return 1;
>  }
>  
> +sub qemu_exec {
> +    my ($vmid, $cmd) = @_;
> +
> +
> +    my $path = shift @$cmd;
> +    my $arguments = $cmd;

maybe name $cmd just $argv? Then the additional variable
wouldn't be needed.

> +
> +    my $args = {
> +	path => $path,
> +	arg => $arguments,
> +	'capture-output' => JSON::true,
> +    };
> +    my $res = PVE::QemuServer::vm_mon_cmd($vmid, "guest-exec", %$args);
> +    raise_agent_error($res, "can't execute command '$path $arguments'");

now when I see the usage I'd rather like another name for this method.
As is, one could think that this raises an (agent) error unconditionally at
first sight.

Could it make sense to have an cmd() method here in this module,
which wraps around vm_mon_cmd which incorporates the error checking?

> +    return $res;
> +}
> +
> +sub qemu_exec_status {
> +    my ($vmid, $pid) = @_;
> +
> +    my $res = PVE::QemuServer::vm_mon_cmd($vmid, "guest-exec-status", pid => $pid );

whitespace error

> +
> +    raise_agent_error($res, "can't get exec statuts for '$pid");
> +
> +    if ($res->{'out-data'}) {
> +	my $decoded = eval { decode_base64($res->{'out-data'}) };
> +	warn $@ if $@;
> +	if (defined($decoded)) {
> +	    $res->{'out-data'} = $decoded;
> +	}
> +    }
> +
> +    if ($res->{'err-data'}) {
> +	my $decoded = eval { decode_base64($res->{'err-data'}) };
> +	warn $@ if $@;
> +	if (defined($decoded)) {
> +	    $res->{'err-data'} = $decoded;
> +	}
> +    }
> +
> +    return $res;
> +}
> +
>  1;
>