Re: [pve-devel] [PATCH qemu-server 2/2] vm start: warn if using ballooning and PCI(e) passthrough

[Friedrich Weber <f.weber@proxmox.com>]

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!