From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id B46F01FF13F for ; Thu, 26 Feb 2026 13:03:59 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 991EF1DFB8; Thu, 26 Feb 2026 13:04:55 +0100 (CET) Message-ID: <041ecf7e-50fd-4929-b0c6-49ca5f9c178e@proxmox.com> Date: Thu, 26 Feb 2026 13:04:51 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH qemu-server v2 1/3] agent: file-read: Allow specifying max number of bytes to read To: Markus Ebner , pve-devel@lists.proxmox.com References: <20260225112851.124188-1-info@ebner-markus.de> <20260225112851.124188-2-info@ebner-markus.de> Content-Language: en-US From: Fiona Ebner In-Reply-To: <20260225112851.124188-2-info@ebner-markus.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1772107473262 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.009 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: NDBSZ2J5AZJO2G4HOUVGGETWWEADQOKQ X-Message-ID-Hash: NDBSZ2J5AZJO2G4HOUVGGETWWEADQOKQ X-MailFrom: f.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 > --- > 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.