From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 6367C1FF15C for ; Fri, 17 Oct 2025 15:36:18 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3CF213E1C; Fri, 17 Oct 2025 15:36:37 +0200 (CEST) Message-ID: Date: Fri, 17 Oct 2025 15:36:02 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion , Daniel Kral References: <20251014143946.160679-1-f.ebner@proxmox.com> <20251014143946.160679-7-f.ebner@proxmox.com> Content-Language: en-US From: Fiona Ebner In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1760708159196 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.023 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH qemu-server 06/16] qmp client: better abstract peer in preparation for qemu-storage-daemon X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Cc: pve-devel Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Am 17.10.25 um 2:39 PM schrieb Daniel Kral: > On Tue Oct 14, 2025 at 4:39 PM CEST, Fiona Ebner wrote: >> @@ -72,21 +70,21 @@ my $push_cmd_to_queue = sub { >> # add a single command to the queue for later execution >> # with queue_execute() >> sub queue_cmd { >> - my ($self, $vmid, $callback, $execute, %params) = @_; >> + my ($self, $peer, $callback, $execute, %params) = @_; >> >> my $cmd = {}; >> $cmd->{execute} = $execute; >> $cmd->{arguments} = \%params; >> $cmd->{callback} = $callback; >> >> - &$push_cmd_to_queue($self, $vmid, $cmd); >> + &$push_cmd_to_queue($self, $peer, $cmd); > > nit: pre-existing but could become a $self->push_cmd_to_queue(...)? Not right now, because $push_cmd_to_queue is a subroutine reference and not a method. And a method can't be private AFAIK. Could be changed to be a private sub and called that way, but it's not in the scope of this patch. >> @@ -158,7 +156,8 @@ sub cmd { >> $self->queue_execute($timeout, 2); >> >> if (defined($queue_info->{error})) { >> - die "VM $vmid qmp command '$cmd->{execute}' failed - $queue_info->{error}" if !$noerr; >> + die "VM $peer->{vmid} $peer->{type} command '$cmd->{execute}' failed - $queue_info->{error}" >> + if !$noerr; > > nit: could be moved into a separate commit as the next patch, but > definitely not important. I'll mention the change in the commit message and squash in the next patch. In a way, stating the type of command is part of the better abstraction. >> @@ -339,7 +336,7 @@ sub queue_execute { >> eval { >> &$open_connection($self, $queue_info, $timeout); >> >> - if (!$queue_info->{qga}) { >> + if ($queue_info->{peer}->{type} ne 'qga') { > > nit: might be worth to add to the patch message that qsd also exposes a > qmp monitor and so will also need to capabilities negotiation, > either in this patch or in the patch introducing qsd. Will do! >> diff --git a/src/PVE/QemuServer/Monitor.pm b/src/PVE/QemuServer/Monitor.pm >> index 0cccdfbe..00b52799 100644 >> --- a/src/PVE/QemuServer/Monitor.pm >> +++ b/src/PVE/QemuServer/Monitor.pm >> @@ -15,20 +15,31 @@ our @EXPORT_OK = qw( >> =head3 qmp_cmd >> >> my $cmd = { execute => $qmp_command_name, arguments => \%params }; >> - my $result = qmp_cmd($vmid, $cmd); >> + my $peer = { vmid => $vmid, type => $type }; >> + my $result = qmp_cmd($peer, $cmd); >> >> -Execute the C<$qmp_command_name> with arguments C<%params> for VM C<$vmid>. Dies if the VM is not >> -running or the monitor socket cannot be reached, even if the C argument is used. Returns the >> -structured result from the QMP side converted from JSON to structured Perl data. In case the >> -C argument is used and the QMP command failed or timed out, the result is a hash reference >> -with an C key containing the error message. >> +Execute the C<$qmp_command_name> with arguments C<%params> for the peer C<$peer>. The type C<$type> >> +of the peer can be C for the QEMU instance of the VM or C for the guest agent of the VM. >> +Dies if the VM is not running or the monitor socket cannot be reached, even if the C argument >> +is used. Returns the structured result from the QMP side converted from JSON to structured Perl >> +data. In case the C argument is used and the QMP command failed or timed out, the result is a >> +hash reference with an C key containing the error message. > > nit: might be enough to state the allowed values for $peer only in the > parameter list below? Even though this won't change a lot, then > there would only be a single source. IMHO, the description should explicitly list what you can communicate with, because it's core part of the functionality and then it's very natural to already mention the type names along with that. It's also adjacent, so it's unlikely to become out of sync after a change. > > nit: asserting that the peer's $type here already would be nice, but > will be done in a later patch anyway. The code after the whole series will end up being the same, but yes, good point, will do! >> @@ -81,7 +93,9 @@ sub mon_cmd { >> >> my $cmd = { execute => $execute, arguments => \%params }; >> >> - return qmp_cmd($vmid, $cmd); >> + my $type = ($execute =~ /^guest\-+/) ? 'qga' : 'qmp'; > > nit: AFAICS $qmpclient->cmd(...) is only used here, but if cmd(...) or > queue_cmd(...) would be used somewhere else, these callers could > set $type = 'qga' and use a qga command. It might be overkill, but > shouldn't that be asserted too? > Sorry, I'm not sure what you mean. Assert that type 'qga' iff $execute =~ /^guest\-+/ in cmd() and queue_cmd()? To be honest, I'd rather try to move away from that in the future and not hard-code it more. For example, introduce and use a qga_cmd() helper. I think it's just pure convention that all guest agent commands start with 'guest-' and that no QMP command starts with 'guest-' yet. If you meant something else, let me know! _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel