[pve-devel] [PATCH qemu-server] fix #2114: set correct link status on hotplug

[Thomas Lamprecht]

On 2/27/19 4:20 PM, Dominik Csapak wrote:
> On 2/27/19 3:53 PM, Thomas Lamprecht wrote:
>> On 2/27/19 2:40 PM, Dominik Csapak wrote:
>>> we also need to set the link status if the whole device changed,
>>> otherwise a change of macaddress allows a network connection even
>>> if link_down is set to 1
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>>> ---
>>>   PVE/QemuServer.pm | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>>> index 6e56eda..3f190a3 100644
>>> --- a/PVE/QemuServer.pm
>>> +++ b/PVE/QemuServer.pm
>>> @@ -4995,6 +4995,7 @@ sub vmconfig_update_net {
>>>         if ($hotplug) {
>>>       vm_deviceplug($storecfg, $conf, $vmid, $opt, $newnet, $arch, $machine_type);
>>> +    qemu_set_link_status($vmid, $opt, !$newnet->{link_down});
>>
>> vm_deviceplug returns undef if it does not succeeds, would it make sense
>> to only do this if it returns truthy?
>>
>> if (vm_deviceplug($storecfg, $conf, $vmid, $opt, $newnet, $arch, $machine_type)) {
>>      qemu_set_link_status($vmid, $opt, !$newnet->{link_down});
>> }
> 
> in that case it would also make sense to have better error recovery?
> or is silently failing ok here?

hmm, reading a bit closer I'm not really sure if the "return undef" case can
happen in the net case, either it works or it dies as "qemu_netdevadd" either
returns straight 1 or it's monitor command can die, in which case the

return undef if !qemu_netdevadd($vmid, ...);

also doesn't triggers (no eval) and thus straight dies. We could still do it
to honor the vm_deviceplug interface...?

> 
>>
>> or move the qemu_set_link_status altogether in vm_deviceplug if branch for 'net'
>> devices?
> 
> that is ok for me, i just wanted to have the places where we set the link together, so that one can see it is done in all necessary cases

OK, I see that reasoning, I just thought that if we have it in there we
won't forget it anymore, on refactoring or new use of the method, and all
info is there readily available. But no strong feelings here, do as it seems
more reasonable to you.

> 
>>
>>>       } else {
>>>       die "skip\n";
>>>       }
>>>
>>