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

[Fiona Ebner <f.ebner@proxmox.com>]

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<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.

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