Re: [pve-devel] [PATCH qemu-server 06/16] qmp client: better abstract peer in preparation for qemu-storage-daemon

["Daniel Kral" <d.kral@proxmox.com>]

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@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") {



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel