[pve-devel] [PATCH container] implement 'edit' command for pct

[Fabian Grünbichler]

I like the general idea, BUT I would really like the following
additions:

- open a temp copy in the editor, not the real one
- flock or digest check before overwriting
- only show/edit the current config by default, with a switch to get the
  full file
- check for parsing errors before moving the changes in place (this is
  less restrictive then the checks for pct/qm set, but better than
  nothing)

the temp file has a number of advantages:
- possible to add some marker on top with explanations, à la git
- misconfigured editors don't wreck havoc in /etc/pve (think: swap
  files, auto-save, ...)
- aborting the edit by removing everything becomes possible (again, like
  git)
- adding any checks is not really possible without anyway

only showing the current config by default limits the potential for
destruction a bit, as old snapshots should almost never be manually
edited anyway.

also see a small comment inline

all of the above of course also applies to 'qm edit' as well ;)

On Mon, Oct 16, 2017 at 03:55:39PM +0200, Dominik Csapak wrote:
> this opens the container config file in the editor which is specified
> via the "EDITOR" environment variable or set via
> update-alternatives (which /usr/bin/editor is a symlink to)
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  src/PVE/CLI/pct.pm | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/src/PVE/CLI/pct.pm b/src/PVE/CLI/pct.pm
> index 3253906..f8a58d0 100755
> --- a/src/PVE/CLI/pct.pm
> +++ b/src/PVE/CLI/pct.pm
> @@ -715,6 +715,34 @@ __PACKAGE__->register_method ({
>  	return undef;
>      }});
>  
> +__PACKAGE__->register_method ({
> +    name => 'edit',
> +    path => 'edit',
> +    method => 'POST',
> +    description => "Opens the CT 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::LXC::complete_vmid_running }),
> +	},
> +    },
> +    returns => { type => 'null'},
> +    code => sub {
> +	my ($param) = @_;
> +
> +	# test if ct exists
> +	my $conf = PVE::LXC::Config->load_config ($param->{vmid});
> +
> +	my $vmid = $param->{vmid};
> +	my $file = "/etc/pve/lxc/$vmid.conf";
> +
> +	my $editor = $ENV{EDITOR} // '/usr/bin/editor';
> +	system("$editor $file");

this won't work as is, and is probably not a good idea anyway. the
environment is considered tainted (so if you actually have $EDITOR set,
this will blow up), and untainting something arbitrary passed to system
like this is potentially problematic in general.

while pct is (currently) limited to root at pam, this would also apply to
systems using sudo to give restricted root access to otherwise
unprivileged accounts. while one could argue that if admins of such
systems allow passing $EDITOR via sudo (which is NOT the default), its
their own fault, the gain is here is very small, and just using
/usr/bin/editor should probably be fine (especially since the edited
files have very simple syntax, so using nano or the like would be
perfectly acceptable for most people).

> +
> +	return undef;
> +    }});
> +
> +
>  our $cmddef = {
>      list=> [ 'PVE::API2::LXC', 'vmlist', [], { node => $nodename }, sub {
>  	my $res = shift;
> @@ -799,6 +827,8 @@ our $cmddef = {
>  
>      cpusets => [ __PACKAGE__, 'cpusets', []],
>  
> +    edit => [ __PACKAGE__, 'edit', ['vmid']],
> +
>  };
>  
>  
> -- 
> 2.11.0
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel