[pve-devel] [PATCH qemu-server 7/8] implement file-read api call via guest-agent

[Thomas Lamprecht]

On 6/7/18 1:16 PM, Dominik Csapak wrote:
> this api call reads a file via the guest agent,
> (in 1MB chunks) but is limited to 16MiB (for now)
> 
> if the file is bigger, the output gets truncated and a
> 'truncated' flag is set in the return object
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  PVE/API2/Qemu/Agent.pm | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/PVE/API2/Qemu/Agent.pm b/PVE/API2/Qemu/Agent.pm
> index ebc5503..dad135c 100644
> --- a/PVE/API2/Qemu/Agent.pm
> +++ b/PVE/API2/Qemu/Agent.pm
> @@ -12,6 +12,9 @@ use JSON;
>  
>  use base qw(PVE::RESTHandler);
>  
> +# max size for file-read over the api
> +my $MAX_READ_SIZE = 16 * 1024 * 1024; # 16 MiB
> +
>  # list of commands
>  # will generate one api endpoint per command
>  # needs a 'method' property and optionally a 'perms' property (default VM.Monitor)
> @@ -114,6 +117,7 @@ __PACKAGE__->register_method({
>  	push @$cmds, qw(
>  	    exec
>  	    exec-status
> +	    file-read
>  	    set-user-password
>  	);
>  
> @@ -335,4 +339,82 @@ __PACKAGE__->register_method({
>  	return { result => $res };
>      }});
>  
> +__PACKAGE__->register_method({
> +    name => 'file-read',
> +    path => 'file-read',
> +    method => 'GET',
> +    protected => 1,
> +    proxyto => 'node',
> +    description => "Reads the given file via guest agent. Is limited to $MAX_READ_SIZE bytes.",
> +    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 }),
> +	    file => {

I'd sync the name for file-path params like used here or in exec,
and personally I'm in favor of 'filename'.
This is used by libc docs for related things which can open
path-like files, like execve. 'pathname' seems also popular
but is more used for calls where the parameter can be either
"normal" file or also directory.
If you don't like those I'm open for what you use here,
just having it consistent would be nice IMO.

> +		type => 'string',
> +		description => 'The path to the file'
> +	    },
> +	},
> +    },
> +    returns => {
> +	type => 'object',
> +	description => "Returns an object with a `content` property.",

Not sure those descriptions are helpful, a reader sees this already
(better) from the properties definition below, or?

> +	properties => {
> +	    content => {
> +		type => 'string',
> +		description => "The content of the file, maximum $MAX_READ_SIZE",
> +	    },
> +	    truncated => {
> +		type => 'boolean',
> +		optional => 1,
> +		description => "If set to 1, the output is truncated and not complete"
> +	    }
> +	},
> +    },
> +    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 $qgafh = PVE::QemuServer::vm_mon_cmd($vmid, "guest-file-open",  path => $param->{file});
> +
> +	raise_agent_error($qgafh, "can't open file");
> +
> +	my $bytes_left = $MAX_READ_SIZE;
> +	my $eof = 0;
> +	my $read_size = 1024*1024;

you got any performance numbers on file transferred through the QGA?
Mostly, can we be (quite) sure that our REST handle synchronous
execution limit does get triggered with this size.
And if, why ever, that still happens the missing close does not
causes problems?

> +	my $content = "";
> +
> +	while ($bytes_left > 0 && !$eof) {
> +	    my $read = PVE::QemuServer::vm_mon_cmd($vmid, "guest-file-read", handle => $qgafh, count => int($read_size));
> +
> +	    raise_agent_error($read, "can't read from file");
> +
> +	    $content .= decode_base64($read->{'buf-b64'});
> +	    $bytes_left -= $read->{count};
> +	    $eof = $read->{eof} // 0;

" // 0" unnecessary?

Most of this could be put in an PVE::QemuServer::Agent::wrapper ?
As done with exec

> +	}
> +
> +	my $res = PVE::QemuServer::vm_mon_cmd($vmid, "guest-file-close", handle => $qgafh);
> +	raise_agent_error($res, "can't close file", 1);
> +
> +	my $result = {
> +	    content => $content
> +	};
> +
> +	if (!$eof) {
> +	    warn "agent file-read: reached maximum read size: $MAX_READ_SIZE bytes. output might be truncated.\n";
> +	    $result->{truncated} = 1;

I'd put the read size in here, i.e., $MAX_READ_SIZE so if
we support seeking at some point an API user can use this
as seek start point for the next request without needing
to know internals of our implementation, which could change,
after all. Another property name would be better then.

> +	}
> +
> +	return $result;
> +    }});
> +
>  1;
>