[pve-devel] [PATCH qemu-server] vzdump: fix log output for disks with iothread

[Thomas Lamprecht]

On 2/27/20 12:01 PM, Stefan Reiter wrote:
> On 2/27/20 11:48 AM, Fabian Grünbichler wrote:
>> On February 26, 2020 11:53 am, Aaron Lauterer wrote:
>>> If IO-Thread is activated and a new enough Qemu version installed the
>>> program still ran into the elsif clause and never in the else clause.
>>> Thus the "include disk ..." log output was missing for these disks.
>>>
>>> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
>>> ---
>>>   PVE/VZDump/QemuServer.pm | 9 +++++----
>>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
>>> index 7695ad6..e36a259 100644
>>> --- a/PVE/VZDump/QemuServer.pm
>>> +++ b/PVE/VZDump/QemuServer.pm
>>> @@ -79,11 +79,12 @@ sub prepare {
>>>       if (defined($drive->{backup}) && !$drive->{backup}) {
>>>           $self->loginfo("exclude disk '$ds' '$volid' (backup=no)");
>>>           return;
>>> -    } elsif ($self->{vm_was_running} && $drive->{iothread}) {
>>> -        if (!PVE::QemuServer::Machine::runs_at_least_qemu_version($vmid, 4, 0, 1)) {
>>> -        die "disk '$ds' '$volid' (iothread=on) can't use backup feature with running QEMU " .
>>> +    } elsif ($self->{vm_was_running}
>>> +         && $drive->{iothread}
>>> +         && !PVE::QemuServer::Machine::runs_at_least_qemu_version($vmid, 4, 0, 1))
>> not new, I know, but:
>>
>> two of those three are not related to the drive. one of those two is
>> actually a qmp call. instead of having a condition spanning three lines,
>> we could refactor it like so:
>>
>> my $iothread_allowed = -1; // to avoid repeated QMP calls
>>
>> foreach_drive(..) {
>>
>> my $iothread = $self->{vm_was_running} && $drive->{iothread}; // do we even care?
>> if ($iothread && $iothread_allowed == -1) {
>>      $iothread_allowed = PVE::QemuServer::Machine::runs_at_least_qemu_version(...); // cache on first use
>> }
>>
>> if ( ... ) {
>>
>> } elsif ($iothread && !$iothread_allowed) { // simple condition ;)
>>    die ...
>> }
>>
>> alternatively, the die could also move into the else branch, assuming we
>> don't have efidisks with iothreads?
>>
>> also, isn't this actually wrong? in stop mode, we'd get the new qemu
>> version when it counts, even if the VM is running the old version right
>> now?
>>
> 
> Doesn't a "stop" mode backup imply the VM being shut down and restarted before backup? I.e. the backup always will run on the currently installed QEMU version?
> 
> If that's the case, this is correct, since we only care about which QEMU version we send the QMP command to.
> 

what's the outcome/state of this?