Thanks for the review! I'll send a v2.
On 14/11/2023 10:13, Fiona Ebner wrote:
> Am 13.11.23 um 18:09 schrieb Friedrich Weber:
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index dbcd568..70983a4 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -5789,6 +5789,16 @@ sub vm_start_nolock {
>> die $err;
>> }
>>
>> + if (
>> + scalar(%$pci_devices)
>
> Style nit: we usually check for scalar(keys ...)
Will change this.
>> + && defined($conf->{balloon})
>> + && $conf->{balloon} != 0
>
> It's nice being explicit, but replacing the above two with just
> $conf->{balloon}, the whole if expression would fit in one line rather
> than six. Either way is fine by me, but the vm_start_nolock function is
> already pretty long.
I opted for being explicit here, but I see the point that the function
is pretty long already and six lines for a relatively unimportant check
is a bit excessive. :) So I'll change this.
> Also might make sense to move the PCI stuff into a
> helper, if you'd like to give it a shot. But not a requirement for this
> patch.
I'll probably keep it at just adding the warning for this patch series.
>
>> + && $conf->{balloon} != $memory
>> + ) {
>> + log_warn("Ballooning is not possible when using PCI(e) passthrough, "
>> + ."VM will use maximum configured memory ($memory MiB).\n");
>
> Trailing newline is not required for log_warn().
>
> Style nit: wrong indentation, we don't usually align text like this. It
> should just be one more indentation for the second line.
Good catches, thanks!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
