[pve-devel] [PATCH qemu-server 06/16] qmp client: better abstract peer in preparation for qemu-storage-daemon
Daniel Kral
d.kral at proxmox.com
Fri Oct 17 14:38:39 CEST 2025
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 <f.ebner at proxmox.com>
> ---
> 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<noerr> argument is used. Returns the
> -structured result from the QMP side converted from JSON to structured Perl data. In case the
> -C<noerr> argument is used and the QMP command failed or timed out, the result is a hash reference
> -with an C<error> 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<qmp> for the QEMU instance of the VM or C<qga> 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<noerr> argument
> +is used. Returns the structured result from the QMP side converted from JSON to structured Perl
> +data. In case the C<noerr> argument is used and the QMP command failed or timed out, the result is a
> +hash reference with an C<error> 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<qmp> for the VM's QEMU instance
> +or C<qga> for the VM's guest agent.
> +
> +=back
> +
> =item C<$cmd>: Hash reference containing the QMP command name for the C<execute> key and additional
> arguments for the QMP command under the C<arguments> 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") {
More information about the pve-devel
mailing list