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.csa...@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@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@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
