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 179A11FF15C for ; Fri, 17 Oct 2025 14:38:55 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5A8342333; Fri, 17 Oct 2025 14:39:14 +0200 (CEST) Mime-Version: 1.0 Date: Fri, 17 Oct 2025 14:38:39 +0200 Message-Id: Cc: "pve-devel" From: "Daniel Kral" To: "Proxmox VE development discussion" X-Mailer: aerc 0.20.0 References: <20251014143946.160679-1-f.ebner@proxmox.com> <20251014143946.160679-7-f.ebner@proxmox.com> In-Reply-To: <20251014143946.160679-7-f.ebner@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1760704716135 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.014 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [qmpclient.pm, snapshot-test.pm, qemuserver.pm, monitor.pm, helpers.pm] 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 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" Only a few nits inline, otherwise a very nice refactor! On Tue Oct 14, 2025 at 4:39 PM CEST, Fiona Ebner wrote: > In preparation to add 'qsd' as a peer type for qemu-storage-daemon. > > There already are two different peer types, namely 'qga' for the QEMU > guest agent and 'qmp' for the QEMU instance itself. > > Signed-off-by: Fiona Ebner > --- > src/PVE/QMPClient.pm | 39 ++++++++++++++++------------------- > src/PVE/QemuServer.pm | 19 ++++++++++------- > src/PVE/QemuServer/Helpers.pm | 6 +++--- > src/PVE/QemuServer/Monitor.pm | 36 ++++++++++++++++++++++---------- > src/PVE/VZDump/QemuServer.pm | 9 ++++++-- > src/test/snapshot-test.pm | 2 +- > 6 files changed, 65 insertions(+), 46 deletions(-) > > diff --git a/src/PVE/QMPClient.pm b/src/PVE/QMPClient.pm > index 1935a336..685cfe81 100644 > --- a/src/PVE/QMPClient.pm > +++ b/src/PVE/QMPClient.pm > @@ -53,15 +53,13 @@ my $qga_allow_close_cmds = { > }; > > my $push_cmd_to_queue = sub { > - my ($self, $vmid, $cmd) = @_; > + my ($self, $peer, $cmd) = @_; > > my $execute = $cmd->{execute} || die "no command name specified"; > > - my $qga = ($execute =~ /^guest\-+/) ? 1 : 0; > + my $sname = PVE::QemuServer::Helpers::qmp_socket($peer); > > - my $sname = PVE::QemuServer::Helpers::qmp_socket($vmid, $qga); > - > - $self->{queue_info}->{$sname} = { qga => $qga, vmid => $vmid, sname => $sname, cmds => [] } > + $self->{queue_info}->{$sname} = { peer => $peer, sname => $sname, cmds => [] } > if !$self->{queue_info}->{$sname}; > > push @{ $self->{queue_info}->{$sname}->{cmds} }, $cmd; > @@ -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(...)? > > return; > } > > # execute a single command > sub cmd { > - my ($self, $vmid, $cmd, $timeout, $noerr) = @_; > + my ($self, $peer, $cmd, $timeout, $noerr) = @_; > > my $result; > > @@ -101,7 +99,7 @@ sub cmd { > $cmd->{callback} = $callback; > $cmd->{arguments} = {} if !defined($cmd->{arguments}); > > - my $queue_info = &$push_cmd_to_queue($self, $vmid, $cmd); > + my $queue_info = &$push_cmd_to_queue($self, $peer, $cmd); nit: same here > > if (!$timeout) { > # hack: monitor sometime blocks > @@ -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. > $result = { error => $queue_info->{error} }; > $result->{'error-is-timeout'} = 1 if $queue_info->{'error-is-timeout'}; > } > @@ -206,10 +205,10 @@ my $open_connection = sub { > > die "duplicate call to open" if defined($queue_info->{fh}); > > - my $vmid = $queue_info->{vmid}; > - my $qga = $queue_info->{qga}; > + my $peer = $queue_info->{peer}; > + my ($vmid, $sotype) = $peer->@{qw(vmid type)}; > > - my $sname = PVE::QemuServer::Helpers::qmp_socket($vmid, $qga); > + my $sname = PVE::QemuServer::Helpers::qmp_socket($peer); > > $timeout = 1 if !$timeout; > > @@ -217,8 +216,6 @@ my $open_connection = sub { > my $starttime = [gettimeofday]; > my $count = 0; > > - my $sotype = $qga ? 'qga' : 'qmp'; > - > for (;;) { > $count++; > $fh = IO::Socket::UNIX->new(Peer => $sname, Blocking => 0, Timeout => 1); > @@ -253,7 +250,7 @@ my $check_queue = sub { > my $fh = $queue_info->{fh}; > next if !$fh; > > - my $qga = $queue_info->{qga}; > + my $qga = $queue_info->{peer}->{type} eq 'qga'; > > if ($queue_info->{error}) { > &$close_connection($self, $queue_info); > @@ -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. > my $cap_cmd = { execute => 'qmp_capabilities', arguments => {} }; > unshift @{ $queue_info->{cmds} }, $cap_cmd; > } > @@ -397,8 +394,8 @@ sub mux_input { > return if !$queue_info; > > my $sname = $queue_info->{sname}; > - my $vmid = $queue_info->{vmid}; > - my $qga = $queue_info->{qga}; > + my $vmid = $queue_info->{peer}->{vmid}; > + my $qga = $queue_info->{peer}->{type} eq 'qga'; > > my $curcmd = $queue_info->{current}; > die "unable to lookup current command for VM $vmid ($sname)\n" if !$curcmd; > @@ -501,8 +498,8 @@ sub mux_eof { > return if !$queue_info; > > my $sname = $queue_info->{sname}; > - my $vmid = $queue_info->{vmid}; > - my $qga = $queue_info->{qga}; > + my $vmid = $queue_info->{peer}->{vmid}; > + my $qga = $queue_info->{peer}->{type} eq 'qga'; > > my $curcmd = $queue_info->{current}; > die "unable to lookup current command for VM $vmid ($sname)\n" if !$curcmd; > diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm > index 45daa06c..7e1b7540 100644 > --- a/src/PVE/QemuServer.pm > +++ b/src/PVE/QemuServer.pm > @@ -2706,13 +2706,15 @@ sub vmstatus { > my $statuscb = sub { > my ($vmid, $resp) = @_; > > - $qmpclient->queue_cmd($vmid, $proxmox_support_cb, 'query-proxmox-support'); > - $qmpclient->queue_cmd($vmid, $blockstatscb, 'query-blockstats'); > - $qmpclient->queue_cmd($vmid, $machinecb, 'query-machines'); > - $qmpclient->queue_cmd($vmid, $versioncb, 'query-version'); > + my $qmp_peer = { vmid => $vmid, type => 'qmp' }; > + > + $qmpclient->queue_cmd($qmp_peer, $proxmox_support_cb, 'query-proxmox-support'); > + $qmpclient->queue_cmd($qmp_peer, $blockstatscb, 'query-blockstats'); > + $qmpclient->queue_cmd($qmp_peer, $machinecb, 'query-machines'); > + $qmpclient->queue_cmd($qmp_peer, $versioncb, 'query-version'); > # this fails if ballon driver is not loaded, so this must be > # the last command (following command are aborted if this fails). > - $qmpclient->queue_cmd($vmid, $ballooncb, 'query-balloon'); > + $qmpclient->queue_cmd($qmp_peer, $ballooncb, 'query-balloon'); > > my $status = 'unknown'; > if (!defined($status = $resp->{'return'}->{status})) { > @@ -2726,7 +2728,8 @@ sub vmstatus { > foreach my $vmid (keys %$list) { > next if $opt_vmid && ($vmid ne $opt_vmid); > next if !$res->{$vmid}->{pid}; # not running > - $qmpclient->queue_cmd($vmid, $statuscb, 'query-status'); > + my $qmp_peer = { vmid => $vmid, type => 'qmp' }; > + $qmpclient->queue_cmd($qmp_peer, $statuscb, 'query-status'); > } > > $qmpclient->queue_execute(undef, 2); > @@ -3180,7 +3183,7 @@ sub config_to_command { > > my $use_virtio = 0; > > - my $qmpsocket = PVE::QemuServer::Helpers::qmp_socket($vmid); > + my $qmpsocket = PVE::QemuServer::Helpers::qmp_socket({ vmid => $vmid, type => 'qmp' }); > push @$cmd, '-chardev', "socket,id=qmp,path=$qmpsocket,server=on,wait=off"; > push @$cmd, '-mon', "chardev=qmp,mode=control"; > > @@ -3417,7 +3420,7 @@ sub config_to_command { > my $guest_agent = parse_guest_agent($conf); > > if ($guest_agent->{enabled}) { > - my $qgasocket = PVE::QemuServer::Helpers::qmp_socket($vmid, 1); > + my $qgasocket = PVE::QemuServer::Helpers::qmp_socket({ vmid => $vmid, type => 'qga' }); > push @$devices, '-chardev', "socket,path=$qgasocket,server=on,wait=off,id=qga0"; > > if (!$guest_agent->{type} || $guest_agent->{type} eq 'virtio') { > diff --git a/src/PVE/QemuServer/Helpers.pm b/src/PVE/QemuServer/Helpers.pm > index 3e444839..cfbcf726 100644 > --- a/src/PVE/QemuServer/Helpers.pm > +++ b/src/PVE/QemuServer/Helpers.pm > @@ -79,9 +79,9 @@ our $var_run_tmpdir = "/var/run/qemu-server"; > mkdir $var_run_tmpdir; > > sub qmp_socket { > - my ($vmid, $qga) = @_; > - my $sockettype = $qga ? 'qga' : 'qmp'; > - return "${var_run_tmpdir}/$vmid.$sockettype"; > + my ($peer) = @_; > + my ($vmid, $type) = $peer->@{qw(vmid type)}; > + return "${var_run_tmpdir}/${vmid}.${type}"; > } > > sub pidfile_name { > 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. nit: asserting that the peer's $type here already would be nice, but will be done in a later patch anyway. > > Parameters: > > =over > > +=item C<$peer>: The peer to communicate with. A hash reference with: > + > +=over > + > =item C<$vmid>: The ID of the virtual machine. > > +=item C<$type>: Type of the peer to communicate with. This can be C for the VM's QEMU instance > +or C for the VM's guest agent. > + > +=back > + > =item C<$cmd>: Hash reference containing the QMP command name for the C key and additional > arguments for the QMP command under the C key. The following custom arguments are not > part of the QMP schema and supported for all commands: > @@ -48,8 +59,9 @@ handle the error that is returned as a structured result. > =cut > > sub qmp_cmd { > - my ($vmid, $cmd) = @_; > + my ($peer, $cmd) = @_; > > + my $vmid = $peer->{vmid}; > my $res; > > my ($noerr, $timeout); > @@ -59,11 +71,11 @@ sub qmp_cmd { > > eval { > die "VM $vmid not running\n" if !PVE::QemuServer::Helpers::vm_running_locally($vmid); > - my $sname = PVE::QemuServer::Helpers::qmp_socket($vmid); > + my $sname = PVE::QemuServer::Helpers::qmp_socket($peer); > if (-e $sname) { # test if VM is reasonably new and supports qmp/qga > my $qmpclient = PVE::QMPClient->new(); > > - $res = $qmpclient->cmd($vmid, $cmd, $timeout, $noerr); > + $res = $qmpclient->cmd($peer, $cmd, $timeout, $noerr); > } else { > die "unable to open monitor socket\n"; > } > @@ -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? > + > + return qmp_cmd({ vmid => $vmid, type => $type }, $cmd); > } > > sub hmp_cmd { > @@ -92,7 +106,7 @@ sub hmp_cmd { > arguments => { 'command-line' => $cmdline, timeout => $timeout }, > }; > > - return qmp_cmd($vmid, $cmd); > + return qmp_cmd({ vmid => $vmid, type => 'qmp' }, $cmd); > } > > 1; > diff --git a/src/PVE/VZDump/QemuServer.pm b/src/PVE/VZDump/QemuServer.pm > index dd789652..6256c0be 100644 > --- a/src/PVE/VZDump/QemuServer.pm > +++ b/src/PVE/VZDump/QemuServer.pm > @@ -995,6 +995,7 @@ sub archive_vma { > } > > my $qmpclient = PVE::QMPClient->new(); > + my $qmp_peer = { vmid => $vmid, type => 'qmp' }; > my $backup_cb = sub { > my ($vmid, $resp) = @_; > $backup_job_uuid = $resp->{return}->{UUID}; > @@ -1012,10 +1013,14 @@ sub archive_vma { > $params->{fleecing} = JSON::true if $task->{'use-fleecing'}; > add_backup_performance_options($params, $opts->{performance}, $qemu_support); > > - $qmpclient->queue_cmd($vmid, $backup_cb, 'backup', %$params); > + $qmpclient->queue_cmd($qmp_peer, $backup_cb, 'backup', %$params); > }; > > - $qmpclient->queue_cmd($vmid, $add_fd_cb, 'getfd', fd => $outfileno, fdname => "backup"); > + $qmpclient->queue_cmd( > + $qmp_peer, $add_fd_cb, 'getfd', > + fd => $outfileno, > + fdname => "backup", > + ); > > my $fs_frozen = $self->qga_fs_freeze($task, $vmid); > > diff --git a/src/test/snapshot-test.pm b/src/test/snapshot-test.pm > index f61cd64b..5808f032 100644 > --- a/src/test/snapshot-test.pm > +++ b/src/test/snapshot-test.pm > @@ -356,7 +356,7 @@ sub vm_running_locally { > # BEGIN mocked PVE::QemuServer::Monitor methods > > sub qmp_cmd { > - my ($vmid, $cmd) = @_; > + my ($peer, $cmd) = @_; > > my $exec = $cmd->{execute}; > if ($exec eq "guest-ping") { _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel