[pve-devel] [PATCH qemu-server v2] implement 'edit' command for qm

[Thomas Lamprecht]

On 10/30/2017 01:52 PM, Dominik Csapak wrote:
> edit the vm config via /usr/bin/editor (this can be set via
> update-alternatives) minus the pending and snapshot sections (we do not
> want to update those manually)
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
> changes from v1:
> * use a tmpfile in /run/VMID.conf.tmp.PID
> * check the digest before and after editing
> * lock config during write part
> * only make the "normal" config editable (no pending and snapshot section)

With the changes this looks quite good, few "non-blocking" comments inline

>  PVE/CLI/qm.pm | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index 90a44ef..66ec779 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -606,6 +606,78 @@ __PACKAGE__->register_method ({
>      }
>  });
>  
> +__PACKAGE__->register_method ({
> +    name => 'edit',
> +    path => 'edit',
> +    method => 'POST',
> +    description => "Opens the VM config file in the editor set via the EDITOR variable or update-alternatives",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::QemuServer::complete_vmid_running }),
> +	},
> +    },
> +    returns => { type => 'null'},
> +    code => sub {
> +	my ($param) = @_;
> +
> +	my $vmid = $param->{vmid};
> +
> +	# load config of vm
> +	my $oldconf = PVE::QemuConfig->load_config($vmid);
> +
> +	# for later check
> +	my $digest = $oldconf->{digest};
> +
> +	# we do not want to edit pending changes or snapshots
> +	delete $oldconf->{pending};

I agree that allowing to edit the pending changes section is not a good idea.
But just removing and then re-applying it on the new, changed config could be
confusing, i.e. if a change to set the cpu cores to 2 is pending, the user edits
the core count to 4 and saves but the next time the machine stop/starts so that
pending gets applied its 2 again.

options I could imagine would be:
* only allow editing stopped VMs and apply an existing pending section before
  opening the editor.
* merge any changed settings with the pending ones if any and the VM is running,
  i.e., if core count change was pending and the user edits the core count the
  change the pending one to the new one?

> +	delete $oldconf->{snapshots};
> +
> +	my $tmppath = "/run/$vmid.conf.tmp.$$";
> +	my $rawconfig = PVE::QemuServer::write_vm_config("/qemu-server/$vmid.conf", $oldconf);

nit, maybe a comment that write_vm_config does not actually writes anything
but just transforms it to the raw config content, the path is "fake" here.

> +
> +	PVE::Tools::file_set_contents($tmppath, $rawconfig);
> +
> +	my $exitcode = system {"/usr/bin/editor"} "/usr/bin/editor", $tmppath;
> +
> +	if ($exitcode == 0) {
> +	    PVE::QemuConfig->lock_config($vmid, sub {
> +
> +		my $raw = PVE::Tools::file_get_contents($tmppath);
> +
> +		# update cluster file system to avoid config cache
> +		PVE::Cluster::cfs_update();
> +		my $conf = PVE::QemuConfig->load_config($vmid);
> +
> +		eval {
> +		    PVE::Tools::assert_if_modified($digest, $conf->{digest});
> +		};
> +
> +		if (my $err = $@) {
> +		    die "$err\n".
> +			"unsaved changes are in: '$tmppath'\n";
> +		}
> +
> +		my $newconf = PVE::QemuServer::parse_vm_config("/qemu-server/$vmid.conf", $raw);

maybe put this also in the eval above?
so that the user also sees where the edited config lies.

> +
> +		# use the pending section and snapshots from the original config
> +		$newconf->{pending} = $conf->{pending};

see my first comment about pending above

> +		$newconf->{snapshots} = $conf->{snapshots};
> +
> +		# write config back
> +		PVE::QemuConfig->write_config($vmid, $newconf);
> +	    });
> +
> +	} else {
> +	    die "/usr/bin/editor exited with '$exitcode' \n";
> +	}
> +

This all could be probably generalized a bit so that it could be reused
for containers?
E.g., adding most of it in guest common as a method which gets the config
class (QemuConfig, LXC::Config), parser methods and the VMID passed as params?
Would be nicer if the raw=>hash and hash=>raw methods were available via the
AbstractConfig derivative classes, but not sure if it's worth to add them just
for this.

> +	# delete tmpfile
> +	unlink($tmppath);
> +	return undef;
> +    }});
> +
> +
>  my $print_agent_result = sub {
>      my ($data) = @_;
>  
> @@ -765,6 +837,8 @@ our $cmddef = {
>  
>      importovf => [ __PACKAGE__, 'importovf', ['vmid', 'manifest', 'storage']],
>  
> +    edit => [ __PACKAGE__, 'edit', ['vmid']],
> +
>  };
>  
>  1;
>