Re: [pve-devel] [PATCH qemu-server 2/3] pci: add helpers to (un)reserve pciids for a vm

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

On 05.10.21 15:11, Dominik Csapak wrote:
> saves a list of pciid <-> vmid mappings in /var/run
> that we can check when we start a vm

a few style nits but also one serious note inline

> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  PVE/QemuServer/PCI.pm | 89 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
> index 5608207..0626b76 100644
> --- a/PVE/QemuServer/PCI.pm
> +++ b/PVE/QemuServer/PCI.pm
> @@ -5,6 +5,7 @@ use strict;
>  
>  use PVE::JSONSchema;
>  use PVE::SysFSTools;
> +use PVE::Tools;
>  
>  use base 'Exporter';
>  
> @@ -461,4 +462,92 @@ sub print_hostpci_devices {
>      return ($kvm_off, $gpu_passthrough, $legacy_igd);
>  }
>  
> +my $PCIID_RESERVATION_FILE = "/var/run/pve-reserved-pciids";
> +my $PCIID_RESERVATION_LCK = "/var/lock/pve-reserved-pciids.lck";
> +
> +my $parse_pci_reservation = sub {
> +    my $pciids = {};
> +
> +    if (my $fh = IO::File->new ($PCIID_RESERVATION_FILE, "r")) {
> +	while (my $line = <$fh>) {
> +	    if ($line =~ m/^($PCIRE)\s(\d+)\s(\d+)$/) {
> +		$pciids->{$1} = {
> +		    timestamp => $2,
> +		    vmid => $3,
> +		};
> +	    }
> +	}
> +    }
> +
> +    return $pciids;
> +};
> +
> +my $write_pci_reservation = sub {
> +    my ($pciids) = @_;
> +
> +    my $data = "";
> +    foreach my $p (keys %$pciids) {

prefer for over foreach

> +	$data .= "$p $pciids->{$p}->{timestamp} $pciids->{$p}->{vmid}\n";
> +    }

my $data = join("\n", map { "$_ $pciids->{$_}->{timestamp} $pciids->{$_}->{vmid}" }, keys $pciids->%*);

> +
> +    PVE::Tools::file_set_contents($PCIID_RESERVATION_FILE, $data);
> +};
> +
> +sub remove_pci_reservation {
> +    my ($id) = @_;
> +
> +    my $code = sub {
> +	my $pciids = $parse_pci_reservation->();
> +
> +	delete $pciids->{$id};
> +
> +	$write_pci_reservation->($pciids);
> +    };
> +
> +    PVE::Tools::lock_file($PCIID_RESERVATION_LCK, 10, $code);

IMO it has some benefit for passing the closure directly, less lines and slightly
more obvious about the locking (as at least I read methods from top to bottom):

PVE::Tools::lock_file($PCIID_RESERVATION_LCK, 10, sub {
    my $pciids = $parse_pci_reservation->();
    ...
});

but we have no clear style guide regarding this and def. use both variants, so no
hard feelings here.

> +    die $@ if $@;
> +
> +    return;
> +}
> +
> +sub reserve_pci_usage {
> +    my ($id, $vmid) = @_;
> +
> +    my $code = sub {
> +
> +	# have a 60 second grace period, since we reserve before
> +	# we actually start the vm

huh, whats the use on that? so I can "steal" PCI devices the first 60s, feels weird...

Why not either:
* catch any start error somewhere centrally and clear the reservation in that
  case again, a kill/crash could still result into false-positives though
* save timestamp now and then once we know it the PID of the VM as third
  param, VMID + PID are quite good in being resistent against PID-reuse
  and an future start could check if the process still lives to decide
  if the reservation is still valid

> +	my $graceperiod = 60;
> +	my $ctime = time();
> +
> +	my $pciids = $parse_pci_reservation->();
> +
> +	if (my $pciid = $pciids->{$id}) {
> +	    if ($pciid->{vmid} == $vmid) {
> +		return; # already reserved
> +	    }

I'd prefer a onliner for above, less lines/noise while not yet being
code-golfy, so easier to read IMO. i.e.:

return if $pciid->{vmid} == $vmid; # already reserved

> +
> +	    if (($pciid->{timestamp} + $graceperiod > $ctime) ||
> +		PVE::QemuServer::Helpers::vm_running_locally($vmid))
> +	    {

style nit², we (nowadays) normally place the if's closing ) also on the new
line:

if (($pciid->{timestamp} + $graceperiod > $ctime) ||
    PVE::QemuServer::Helpers::vm_running_locally($vmid)
) {
    ....
}

honestly I'd like it 1000% more the rust way, but well..