[pve-devel] [PATCH qemu-server 1/1] add hookscripts to vms

[Fabian Grünbichler]

On Wed, Jan 23, 2019 at 03:34:56PM +0100, Dominik Csapak wrote:
> On 1/23/19 3:27 PM, Fabian Grünbichler wrote:
> > On Mon, Jan 21, 2019 at 09:44:35AM +0100, Dominik Csapak wrote:
> > > this adds a new config option for it, and executes it on four
> > > points in time:
> > > 
> > > 'pre-start'
> > > 'post-start'
> > > 'pre-stop'
> > > 'post-stop'
> > > 
> > > on pre-start we abort if the script fails
> > > and pre-stop will not be called if the vm crashes or if
> > > the vm gets powered off from inside the guest
> > > 
> > > Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> > > ---
> > >   PVE/API2/Qemu.pm  |  8 ++++++++
> > >   PVE/CLI/qm.pm     |  2 ++
> > >   PVE/QemuServer.pm | 12 ++++++++++++
> > >   3 files changed, 22 insertions(+)
> > > 
> > > diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> > > index b55fd13..13aa021 100644
> > > --- a/PVE/API2/Qemu.pm
> > > +++ b/PVE/API2/Qemu.pm
> > > @@ -1101,6 +1101,14 @@ my $update_vm_api  = sub {
> > >   	    if ($param->{$opt} eq '1') {
> > >   		$param->{$opt} = PVE::QemuServer::generate_uuid();
> > >   	    }
> > > +	} elsif ($opt eq 'hookscript') {
> > > +	    my ($path, undef, $type) = PVE::Storage::path($storecfg, $param->{$opt});
> > 
> > IMHO this should be limited to root at pam here as well (just because an
> > admin put an executable in some scripts directory does not mean they
> > want every user that can modify their VM config to execute that script
> > as root - potentially that script then decides stuff based on VM config
> > content as well, etc. pp.). once we establish a ACL scheme for this
> > scripts we might relax this here, but for now let's keep it simple &
> > safe by default.
> 
> any new config option is by default root at pam only
> (see PVE/API2/Qemu.pm:335)
> 
> so this is already done?

fair enough - as discussed off-disk ;)

> > > +	    raise_param_exc({ $opt => "is not in the scripts directory" })
> > > +		if $type ne 'scripts';
> > > +
> > > +	    warn "script '$path' not found, setting anyway\n"
> > > +		if ! -f $path;
> > 
> > does this make sense?
> 
> what exactly? i did not want the user to be able to set anything
> (e.g. 'local:5' which is a valid volume-id but nonsense in this
> context), but i did not want the user to be restricted
> to first put the script in the storage and then set it

sorry - not failing if the hook script does not exist. we don't allow
adding non-existing volumes as disks either, and IMHO the usual
assumption would be that referencing a hook script to be executed which
does not exist is an error (e.g., a typo, or something like that).