Re: [PATCH qemu-server v2 1/3] agent: file-read: Allow specifying max number of bytes to read

[Fiona Ebner <f.ebner@proxmox.com>]

Am 25.02.26 um 1:06 PM schrieb Markus Ebner:
> The previous file-read implementation in the Proxmox API always
> attempted to read a fixed 16 MiB. On busy or resource-constrainted
> hosts, that often caused request timeouts, with no option to
> reduce the number of read bytes to at least read something.
> 
> The new parameter count now allows specifying the exact number of
> bytes that should be read from the given file. To be backwards
> compatible with the previous behavior, it defaults to 16 MiB.
> These 16 MiB are further also the maximum allowed value.
> 
> Signed-off-by: Markus Ebner <info@ebner-markus.de>
> ---
>  src/PVE/API2/Qemu/Agent.pm | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/src/PVE/API2/Qemu/Agent.pm b/src/PVE/API2/Qemu/Agent.pm
> index de36ce1e..f32e1dc2 100644
> --- a/src/PVE/API2/Qemu/Agent.pm
> +++ b/src/PVE/API2/Qemu/Agent.pm
> @@ -464,6 +464,14 @@ __PACKAGE__->register_method({
>                  'pve-vmid',
>                  { completion => \&PVE::QemuServer::complete_vmid_running },
>              ),
> +            count => {
> +                type => 'integer',
> +                optional => 1,
> +                minimum => 0,

Is allowing 0 intentional?

> +                maximum => $MAX_READ_SIZE,
> +                default => $MAX_READ_SIZE,
> +                description => "Number of bytes to read.",
> +            },
>              file => {
>                  type => 'string',
>                  description => 'The path to the file',
> @@ -487,6 +495,7 @@ __PACKAGE__->register_method({
>      },
>      code => sub {
>          my ($param) = @_;
> +        my $count = int($param->{count} // $MAX_READ_SIZE);

Nit: The int() is superfluous.

>  
>          my $vmid = $param->{vmid};
>          my $conf = PVE::QemuConfig->load_config($vmid);
> @@ -494,18 +503,20 @@ __PACKAGE__->register_method({
>          my $qgafh =
>              agent_cmd($vmid, $conf, "file-open", { path => $param->{file} }, "can't open file");
>  
> -        my $bytes_left = $MAX_READ_SIZE;
> +        my $bytes_read = 0;
>          my $eof = 0;
>          my $read_size = 1024 * 1024;
>          my $content = "";
>  
> -        while ($bytes_left > 0 && !$eof) {
> +        while ($bytes_read < $count && !$eof) {
> +            my $bytes_left = $count - $bytes_read;
> +            my $chunk_size = $bytes_left < $read_size ? $bytes_left : $read_size;
>              my $read =
> -                mon_cmd($vmid, "guest-file-read", handle => $qgafh, count => int($read_size));
> +                mon_cmd($vmid, "guest-file-read", handle => $qgafh, count => int($chunk_size));
>              check_agent_error($read, "can't read from file");
>  
>              $content .= decode_base64($read->{'buf-b64'});
> -            $bytes_left -= $read->{count};
> +            $bytes_read += $read->{count};
>              $eof = $read->{eof} // 0;
>          }
>  
> @@ -514,12 +525,14 @@ __PACKAGE__->register_method({
>  
>          my $result = {
>              content => $content,
> -            'bytes-read' => ($MAX_READ_SIZE - $bytes_left),
> +            'bytes-read' => $bytes_read,
>          };
>  
>          if (!$eof) {
> -            warn
> -                "agent file-read: reached maximum read size: $MAX_READ_SIZE bytes. output might be truncated.\n";
> +            if (!defined $param->{count}) {

Style nit: please use parentheses for definedness checks:
if (!defined($param->{count})) {

> +                warn "agent file-read: reached maximum read size: $MAX_READ_SIZE bytes."
> +                    . " output might be truncated.\n";
> +            }
>              $result->{truncated} = 1;

The description for 'truncated' is:
"If set to 1, the output is truncated and not complete"

But if I request 1 byte and get 1 byte, why should it be considered
"truncated and not complete"?

I do think it's useful information to have 'truncated' for the API
users, but the description should be updated to better convey that it's
about whether EOF was reached or not.