Re: [pve-devel] [PATCH v5 qemu-server 07/11] mtunnel: add API endpoints

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On February 11, 2022 2:01 pm, Fabian Ebner wrote:
> Am 09.02.22 um 14:07 schrieb Fabian Grünbichler:
>> +	PVE::Cluster::check_cfs_quorum();
>> +
>> +	my $socket_addr = "/run/qemu-server/$vmid.mtunnel";
> 
> Nit: since there is another variable with the same name inside $realcmd
> below, and this one is not used until the end, it could be moved further
> down.

ack

> 
>> +
>> +	my $lock = 'create';
>> +	eval { PVE::QemuConfig->create_and_lock_config($vmid, 0, $lock); };
>> +
>> +	raise_param_exc({ vmid => "unable to create empty VM config - $@"})
>> +	    if $@;
>> +
>> +	my $realcmd = sub {
> 
> ----8<----
> 
>> +		'config' => sub {
>> +		    my ($params) = @_;
>> +
> 
> ----8<----
> 
>> +		    # not handled by update_vm_api
>> +		    my $vmgenid = delete $new_conf->{vmgenid};
>> +		    my $meta = delete $new_conf->{meta};
>> +
>> +		    $new_conf->{vmid} = $state->{vmid};
>> +		    $new_conf->{node} = $node;
>> +
>> +		    PVE::QemuConfig->remove_lock($state->{vmid}, 'create');
>> +
>> +		    eval {
>> +			$update_vm_api->($new_conf, 1);
>> +		    };
>> +		    if (my $err = $@) {
>> +			# revert to locked previous config
>> +			my $conf = PVE::QemuConfig->load_config($state->{vmid});
>> +			$conf->{lock} = 'create';
>> +			PVE::QemuConfig->write_config($state->{vmid}, $conf);
>> +
>> +			die $err;
>> +		    }
>> +
>> +		    my $conf = PVE::QemuConfig->load_config($state->{vmid});
>> +		    $conf->{lock} = 'migrate';
>> +		    $conf->{vmgenid} = $vmgenid if $vmgenid;
>> +		    $conf->{meta} = $meta if $meta;
> 
> Nit: shouldn't matter for these two, but 'if defined(...)' feels safer
> 
>> +		    PVE::QemuConfig->write_config($state->{vmid}, $conf);
>> +
>> +		    $state->{lock} = 'migrate';
>> +
>> +		    return;
>> +		},
> 
> ----8<----
> 
>> +		'resume' => sub {
>> +		    if (PVE::QemuServer::check_running($state->{vmid}, 1)) {
> 
> Nit: use PVE::QemuServer::Helpers::vm_running_locally instead, as the
> comment above PVE::QemuServer::check_running suggests

done. this one might be worthy of a repo-wide cleanup at some point?

> 
>> +			PVE::QemuServer::vm_resume($state->{vmid}, 1, 1);
>> +		    } else {
>> +			die "VM $state->{vmid} not running\n";
>> +		    }
>> +		    return;
>> +		},
>