Re: [pve-devel] [PATCH qemu-server 2/2] backup: use dirty-bitmap for PBS only if supported by QEMU

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 08.07.20 11:22, Stefan Reiter wrote:
> On 7/8/20 11:08 AM, Thomas Lamprecht wrote:
>> On 08.07.20 10:42, Stefan Reiter wrote:
>>> If 'query-proxmox-support' is not known to QEMU, assume that no other
>>> features are supported either.
>>>
>>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>>> ---
>>>
>>> Does not require any version bumps, since it handles failure gracefully.
>>>
>>>   PVE/VZDump/QemuServer.pm | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
>>> index 567088c..3c2a334 100644
>>> --- a/PVE/VZDump/QemuServer.pm
>>> +++ b/PVE/VZDump/QemuServer.pm
>>> @@ -443,6 +443,8 @@ sub archive_pbs {
>>>           die "interrupted by signal\n";
>>>       };
>>>   +    my $qemu_support = eval { mon_cmd($vmid, "query-proxmox-support") } // {};
>>
>> can we not fall back and instead use undef here to error out with a nice
>> "does not support PBS at all" message?
>>
> 
> If we want that, sure. Though I think it might be cleaner to also have a 'pbs' feature as well, I don't like the approach of implicitly detecting support just because a QMP command exists.

adding an extra command which plainly returns true doesn't changes anything,
at least when done in the initial version of this as then it only can go:

* qmp command exists -> always true
* qmp command doesn'T exists -> always false

so you do not gain anything by that, but I do not care to much..

> 
> I'll send a v2.
> 
>>> +
>>>       my $fs_frozen = $self->qga_fs_freeze($task, $vmid);
>>>         my $params = {
>>> @@ -453,11 +455,13 @@ sub archive_pbs {
>>>           password => $password,
>>>           devlist => $devlist,
>>>           'config-file' => $conffile,
>>> -        'use-dirty-bitmap' => JSON::true,
>>>       };
>>>       $params->{fingerprint} = $fingerprint if defined($fingerprint);
>>>       $params->{'firewall-file'} = $firewall if -e $firewall;
>>>   +    $params->{'use-dirty-bitmap'} = JSON::true
>>> +        if $qemu_support->{'pbs-dirty-bitmap'};
>>> +
>>>       $params->{timeout} = 60; # give some time to connect to the backup server
>>>         my $res = eval { mon_cmd($vmid, "backup", %$params) };
>>>
>>