]> git.proxmox.com Git - pve-container.git/blobdiff - src/PVE/LXC.pm
Refactor has_feature
[pve-container.git] / src / PVE / LXC.pm
index 534a43c222e008cdda8959ad1471613b16a21076..503a57fb872ada093e83b101349afe851950baf3 100644 (file)
@@ -9,14 +9,15 @@ use Socket;
 use File::Path;
 use File::Spec;
 use Cwd qw();
-use Fcntl qw(O_RDONLY :flock);
+use Fcntl qw(O_RDONLY);
 
 use PVE::Cluster qw(cfs_register_file cfs_read_file);
+use PVE::Exception qw(raise_perm_exc);
 use PVE::Storage;
 use PVE::SafeSyslog;
 use PVE::INotify;
 use PVE::JSONSchema qw(get_standard_option);
-use PVE::Tools qw($IPV6RE $IPV4RE dir_glob_foreach);
+use PVE::Tools qw($IPV6RE $IPV4RE dir_glob_foreach lock_file lock_file_full);
 use PVE::Network;
 use PVE::AccessControl;
 use PVE::ProcFSTools;
@@ -119,8 +120,8 @@ my $confdesc = {
     ostype => {
        optional => 1,
        type => 'string',
-       enum => ['debian', 'ubuntu', 'centos', 'fedora', 'opensuse', 'archlinux'],
-       description => "OS type. Corresponds to lxc setup scripts in /usr/share/lxc/config/<ostype>.common.conf.",
+       enum => ['debian', 'ubuntu', 'centos', 'fedora', 'opensuse', 'archlinux', 'alpine', 'unmanaged'],
+       description => "OS type. This is used to setup configuration inside the container, and corresponds to lxc setup scripts in /usr/share/lxc/config/<ostype>.common.conf. Value 'unmanaged' can be used to skip and OS specific setup.",
     },
     console => {
        optional => 1,
@@ -646,28 +647,46 @@ sub write_config {
 my $lock_handles =  {};
 my $lockdir = "/run/lock/lxc";
 
-sub lock_filename {
+sub config_file_lock {
     my ($vmid) = @_;
 
     return "$lockdir/pve-config-${vmid}.lock";
 }
 
-sub lock_container {
+sub lock_config_full {
     my ($vmid, $timeout, $code, @param) = @_;
 
-    $timeout = 10 if !$timeout;
+    my $filename = config_file_lock($vmid);
 
-    my $filename = lock_filename($vmid);
+    mkdir $lockdir if !-d $lockdir;
+
+    my $res = lock_file($filename, $timeout, $code, @param);
+
+    die $@ if $@;
+
+    return $res;
+}
+
+sub lock_config_mode {
+    my ($vmid, $timeout, $shared, $code, @param) = @_;
+
+    my $filename = config_file_lock($vmid);
 
     mkdir $lockdir if !-d $lockdir;
 
-    my $res = PVE::Tools::lock_file_full($filename, $timeout, 0, $code, @param);
+    my $res = lock_file_full($filename, $timeout, $shared, $code, @param);
 
     die $@ if $@;
 
     return $res;
 }
 
+sub lock_config {
+    my ($vmid, $code, @param) = @_;
+
+    return lock_config_full($vmid, 10, $code, @param);
+}
+
 sub option_exists {
     my ($name) = @_;
 
@@ -1064,10 +1083,14 @@ sub update_lxc_config {
     my $custom_idmap = grep { $_->[0] eq 'lxc.id_map' } @{$conf->{lxc}};
 
     my $ostype = $conf->{ostype} || die "missing 'ostype' - internal error";
-    if ($ostype =~ /^(?:debian | ubuntu | centos | fedora | opensuse | archlinux)$/x) {
-       $raw .= "lxc.include = /usr/share/lxc/config/$ostype.common.conf\n";
+    if ($ostype =~ /^(?:debian | ubuntu | centos | fedora | opensuse | archlinux | alpine | unmanaged)$/x) {
+       my $inc ="/usr/share/lxc/config/$ostype.common.conf";
+       $inc ="/usr/share/lxc/config/common.conf" if !-f $inc;
+       $raw .= "lxc.include = $inc\n";
        if ($unprivileged || $custom_idmap) {
-           $raw .= "lxc.include = /usr/share/lxc/config/$ostype.userns.conf\n"
+           $inc = "/usr/share/lxc/config/$ostype.userns.conf";
+           $inc = "/usr/share/lxc/config/userns.conf" if !-f $inc;
+           $raw .= "lxc.include = $inc\n"
        }
     } else {
        die "implement me (ostype $ostype)";
@@ -1175,6 +1198,28 @@ sub verify_searchdomain_list {
     return join(' ', @list);
 }
 
+sub is_volume_in_use {
+    my ($config, $volid, $include_snapshots) = @_;
+    my $used = 0;
+
+    foreach_mountpoint($config, sub {
+       my ($ms, $mountpoint) = @_;
+       return if $used;
+       if ($mountpoint->{type} eq 'volume' && $mountpoint->{volume} eq $volid) {
+           $used = 1;
+       }
+    });
+
+    my $snapshots = $config->{snapshots};
+    if ($include_snapshots && $snapshots) {
+       foreach my $snap (keys %$snapshots) {
+           $used ||= is_volume_in_use($snapshots->{$snap}, $volid);
+       }
+    }
+
+    return $used;
+}
+
 sub add_unused_volume {
     my ($config, $volid) = @_;
 
@@ -1251,11 +1296,11 @@ sub update_pct_config {
            } elsif ($opt =~ m/^mp(\d+)$/) {
                next if $hotplug_error->($opt);
                check_protection($conf, "can't remove CT $vmid drive '$opt'");
-               my $mountpoint = parse_ct_mountpoint($conf->{$opt});
-               if ($mountpoint->{type} eq 'volume') {
-                   add_unused_volume($conf, $mountpoint->{volume})
-               }
+               my $mp = parse_ct_mountpoint($conf->{$opt});
                delete $conf->{$opt};
+               if ($mp->{type} eq 'volume' && !is_volume_in_use($conf, $mp->{volume})) {
+                   add_unused_volume($conf, $mp->{volume});
+               }
            } elsif ($opt eq 'unprivileged') {
                die "unable to delete read-only option: '$opt'\n";
            } else {
@@ -1294,6 +1339,8 @@ sub update_pct_config {
        write_config($vmid, $conf) if $running;
     }
 
+    my $used_volids = {};
+
     foreach my $opt (keys %$param) {
        my $value = $param->{$opt};
        if ($opt eq 'hostname') {
@@ -1334,23 +1381,59 @@ sub update_pct_config {
         } elsif ($opt =~ m/^mp(\d+)$/) {
            next if $hotplug_error->($opt);
            check_protection($conf, "can't update CT $vmid drive '$opt'");
+           my $old = $conf->{$opt};
            $conf->{$opt} = $value;
+           if (defined($old)) {
+               my $mp = parse_ct_mountpoint($old);
+               if ($mp->{type} eq 'volume' && !is_volume_in_use($conf, $mp->{volume})) {
+                   add_unused_volume($conf, $mp->{volume});
+               }
+           }
            $new_disks = 1;
+           my $mp = parse_ct_mountpoint($value);
+           $used_volids->{$mp->{volume}} = 1;
         } elsif ($opt eq 'rootfs') {
            next if $hotplug_error->($opt);
            check_protection($conf, "can't update CT $vmid drive '$opt'");
+           my $old = $conf->{$opt};
            $conf->{$opt} = $value;
+           if (defined($old)) {
+               my $mp = parse_ct_rootfs($old);
+               if ($mp->{type} eq 'volume' && !is_volume_in_use($conf, $mp->{volume})) {
+                   add_unused_volume($conf, $mp->{volume});
+               }
+           }
+           my $mp = parse_ct_rootfs($value);
+           $used_volids->{$mp->{volume}} = 1;
        } elsif ($opt eq 'unprivileged') {
            die "unable to modify read-only option: '$opt'\n";
+       } elsif ($opt eq 'ostype') {
+           next if $hotplug_error->($opt);
+           $conf->{$opt} = $value;
        } else {
            die "implement me: $opt";
        }
        write_config($vmid, $conf) if $running;
     }
 
+    # Cleanup config:
+
+    # Remove unused disks after re-adding
+    foreach my $key (keys %$conf) {
+       next if $key !~ /^unused\d+/;
+       my $volid = $conf->{$key};
+       if ($used_volids->{$volid}) {
+           delete $conf->{$key};
+       }
+    }
+
+    # Apply deletions and creations of new volumes
     if (@deleted_volumes) {
        my $storage_cfg = PVE::Storage::config();
        foreach my $volume (@deleted_volumes) {
+           next if $used_volids->{$volume}; # could have been re-added, too
+           # also check for references in snapshots
+           next if is_volume_in_use($conf, $volume, 1);
            delete_mountpoint_volume($storage_cfg, $vmid, $volume);
        }
     }
@@ -1692,13 +1775,37 @@ my $snapshot_copy_config = sub {
        next if $k eq 'lock';
        next if $k eq 'digest';
        next if $k eq 'description';
+       next if $k =~ m/^unused\d+$/;
 
        $dest->{$k} = $source->{$k};
     }
 };
 
-my $snapshot_prepare = sub {
-    my ($vmid, $snapname, $comment) = @_;
+my $snapshot_apply_config = sub {
+    my ($conf, $snap) = @_;
+
+    # copy snapshot list
+    my $newconf = {
+       snapshots => $conf->{snapshots},
+    };
+
+    # keep description and list of unused disks
+    foreach my $k (keys %$conf) {
+       next if !($k =~ m/^unused\d+$/ || $k eq 'description');
+       $newconf->{$k} = $conf->{$k};
+    }
+
+    &$snapshot_copy_config($snap, $newconf);
+
+    return $newconf;
+};
+
+sub snapshot_save_vmstate {
+    die "implement me - snapshot_save_vmstate\n";
+}
+
+sub snapshot_prepare {
+    my ($vmid, $snapname, $save_vmstate, $comment) = @_;
 
     my $snap;
 
@@ -1717,27 +1824,30 @@ my $snapshot_prepare = sub {
            if defined($conf->{snapshots}->{$snapname});
 
        my $storecfg = PVE::Storage::config();
-       my $feature = $snapname eq 'vzdump' ? 'vzdump' : 'snapshot';
-       die "snapshot feature is not available\n" if !has_feature($feature, $conf, $storecfg);
+       die "snapshot feature is not available\n"
+           if !has_feature('snapshot', $conf, $storecfg, undef, undef, $snapname eq 'vzdump');
 
        $snap = $conf->{snapshots}->{$snapname} = {};
 
+       if ($save_vmstate && check_running($vmid)) {
+           snapshot_save_vmstate($vmid, $conf, $snapname, $storecfg);
+       }
+
        &$snapshot_copy_config($conf, $snap);
 
-       $snap->{'snapstate'} = "prepare";
-       $snap->{'snaptime'} = time();
-       $snap->{'description'} = $comment if $comment;
-       $conf->{snapshots}->{$snapname} = $snap;
+       $snap->{snapstate} = "prepare";
+       $snap->{snaptime} = time();
+       $snap->{description} = $comment if $comment;
 
        write_config($vmid, $conf);
     };
 
-    lock_container($vmid, 10, $updatefn);
+    lock_config($vmid, $updatefn);
 
     return $snap;
-};
+}
 
-my $snapshot_commit = sub {
+sub snapshot_commit {
     my ($vmid, $snapname) = @_;
 
     my $updatefn = sub {
@@ -1747,41 +1857,37 @@ my $snapshot_commit = sub {
        die "missing snapshot lock\n"
            if !($conf->{lock} && $conf->{lock} eq 'snapshot');
 
-       die "snapshot '$snapname' does not exist\n"
-           if !defined($conf->{snapshots}->{$snapname});
+       my $snap = $conf->{snapshots}->{$snapname};
+       die "snapshot '$snapname' does not exist\n" if !defined($snap);
 
        die "wrong snapshot state\n"
-           if !($conf->{snapshots}->{$snapname}->{'snapstate'} && 
-                $conf->{snapshots}->{$snapname}->{'snapstate'} eq "prepare");
+           if !($snap->{snapstate} && $snap->{snapstate} eq "prepare");
 
-       delete $conf->{snapshots}->{$snapname}->{'snapstate'};
+       delete $snap->{snapstate};
        delete $conf->{lock};
-       $conf->{parent} = $snapname;
 
-       write_config($vmid, $conf);
+       my $newconf = &$snapshot_apply_config($conf, $snap);
+
+       $newconf->{parent} = $snapname;
+
+       write_config($vmid, $newconf);
     };
 
-    lock_container($vmid, 10 ,$updatefn);
-};
+    lock_config($vmid, $updatefn);
+}
 
 sub has_feature {
-    my ($feature, $conf, $storecfg, $snapname) = @_;
+    my ($feature, $conf, $storecfg, $snapname, $running, $backup_only) = @_;
     
     my $err;
-    my $vzdump = $feature eq 'vzdump';
-    $feature = 'snapshot' if $vzdump;
 
     foreach_mountpoint($conf, sub {
        my ($ms, $mountpoint) = @_;
 
        return if $err; # skip further test
-       return if $vzdump && $ms ne 'rootfs' && !$mountpoint->{backup};
+       return if $backup_only && $ms ne 'rootfs' && !$mountpoint->{backup};
        
-       $err = 1 if !PVE::Storage::volume_has_feature($storecfg, $feature, $mountpoint->{volume}, $snapname);
-
-       # TODO: implement support for mountpoints
-       die "unable to handle mountpoint '$ms' - feature not implemented\n"
-           if $ms ne 'rootfs';
+       $err = 1 if !PVE::Storage::volume_has_feature($storecfg, $feature, $mountpoint->{volume}, $snapname, $running);
     });
 
     return $err ? 0 : 1;
@@ -1856,83 +1962,70 @@ sub sync_container_namespace {
     die "failed to sync container namespace\n" if $? != 0;
 }
 
+sub check_freeze_needed {
+    my ($vmid, $config, $save_vmstate) = @_;
+
+    my $ret = check_running($vmid);
+    return ($ret, $ret);
+}
+
 sub snapshot_create {
-    my ($vmid, $snapname, $comment) = @_;
+    my ($vmid, $snapname, $save_vmstate, $comment) = @_;
+
+    my $snap = snapshot_prepare($vmid, $snapname, $save_vmstate, $comment);
 
-    my $snap = &$snapshot_prepare($vmid, $snapname, $comment);
+    $save_vmstate = 0 if !$snap->{vmstate};
 
     my $conf = load_config($vmid);
 
-    my $running = check_running($vmid);
-    
-    my $unfreeze = 0;
+    my ($running, $freezefs) = check_freeze_needed($vmid, $conf, $snap->{vmstate});
 
     my $drivehash = {};
 
     eval {
-       if ($running) {
-           $unfreeze = 1;
+       if ($freezefs) {
            PVE::Tools::run_command(['/usr/bin/lxc-freeze', '-n', $vmid]);
            sync_container_namespace($vmid);
-       };
+       }
 
        my $storecfg = PVE::Storage::config();
-       my $rootinfo = parse_ct_rootfs($conf->{rootfs});
-       my $volid = $rootinfo->{volume};
+       foreach_mountpoint($conf, sub {
+           my ($ms, $mountpoint) = @_;
 
-       PVE::Storage::volume_snapshot($storecfg, $volid, $snapname);
-       $drivehash->{rootfs} = 1;
+           return if $snapname eq 'vzdump' && $ms ne 'rootfs' && !$mountpoint->{backup};
+           PVE::Storage::volume_snapshot($storecfg, $mountpoint->{volume}, $snapname);
+           $drivehash->{$ms} = 1;
+       });
     };
     my $err = $@;
     
-    if ($unfreeze) {
-       eval { PVE::Tools::run_command(['/usr/bin/lxc-unfreeze', '-n', $vmid]); };
-       warn $@ if $@;
+    if ($running) {
+       if ($freezefs) {
+           eval { PVE::Tools::run_command(['/usr/bin/lxc-unfreeze', '-n', $vmid]); };
+           warn $@ if $@;
+       }
     }
     
     if ($err) {
+       warn "snapshot create failed: starting cleanup\n";
        eval { snapshot_delete($vmid, $snapname, 1, $drivehash); };
-       warn "$@\n" if $@;
+       warn "$@" if $@;
        die "$err\n";
     }
 
-    &$snapshot_commit($vmid, $snapname);
+    snapshot_commit($vmid, $snapname);
 }
 
 # Note: $drivehash is only set when called from snapshot_create.
 sub snapshot_delete {
     my ($vmid, $snapname, $force, $drivehash) = @_;
 
-    my $snap;
-
-    my $conf;
+    my $prepare = 1;
 
-    my $updatefn =  sub {
-
-       $conf = load_config($vmid);
-
-       die "you can't delete a snapshot if vm is a template\n"
-           if is_template($conf);
-
-       $snap = $conf->{snapshots}->{$snapname};
-
-       if (!$drivehash) {
-           check_lock($conf);
-       }
-
-       die "snapshot '$snapname' does not exist\n" if !defined($snap);
-
-       $snap->{snapstate} = 'delete';
-
-       write_config($vmid, $conf);
-    };
-
-    lock_container($vmid, 10, $updatefn);
-
-    my $storecfg = PVE::Storage::config();
+    my $snap;
+    my $unused = [];
 
     my $unlink_parent = sub {
-
        my ($confref, $new_parent) = @_;
 
        if ($confref->{parent} && $confref->{parent} eq $snapname) {
@@ -1944,102 +2037,167 @@ sub snapshot_delete {
        }
     };
 
-    my $del_snap =  sub {
+    my $updatefn =  sub {
+       my ($remove_drive) = @_;
 
-       $conf = load_config($vmid);
+       my $conf = load_config($vmid);
 
-       if ($drivehash) {
-           delete $conf->{lock};
-       } else {
+       if (!$drivehash) {
            check_lock($conf);
+           die "you can't delete a snapshot if vm is a template\n"
+               if is_template($conf);
        }
 
-       my $parent = $conf->{snapshots}->{$snapname}->{parent};
-       foreach my $snapkey (keys %{$conf->{snapshots}}) {
-           &$unlink_parent($conf->{snapshots}->{$snapkey}, $parent);
+       $snap = $conf->{snapshots}->{$snapname};
+
+       die "snapshot '$snapname' does not exist\n" if !defined($snap);
+
+       # remove parent refs
+       if (!$prepare) {
+           &$unlink_parent($conf, $snap->{parent});
+           foreach my $sn (keys %{$conf->{snapshots}}) {
+               next if $sn eq $snapname;
+               &$unlink_parent($conf->{snapshots}->{$sn}, $snap->{parent});
+           }
        }
 
-       &$unlink_parent($conf, $parent);
+       if ($remove_drive) {
+           if ($remove_drive eq 'vmstate') {
+               die "implement me - saving vmstate\n";
+           } else {
+               my $value = $snap->{$remove_drive};
+               my $mountpoint = $remove_drive eq 'rootfs' ? parse_ct_rootfs($value, 1) : parse_ct_mountpoint($value, 1);
+               delete $snap->{$remove_drive};
+               add_unused_volume($snap, $mountpoint->{volume})
+                   if (!is_volume_in_use($snap, $mountpoint->{volume}));
+           }
+       }
 
-       delete $conf->{snapshots}->{$snapname};
+       if ($prepare) {
+           $snap->{snapstate} = 'delete';
+       } else {
+           delete $conf->{snapshots}->{$snapname};
+           delete $conf->{lock} if $drivehash;
+           foreach my $volid (@$unused) {
+               add_unused_volume($conf, $volid)
+                   if (!is_volume_in_use($conf, $volid));
+           }
+       }
 
        write_config($vmid, $conf);
     };
 
-    my $rootfs = $conf->{snapshots}->{$snapname}->{rootfs};
-    my $rootinfo = parse_ct_rootfs($rootfs);
-    my $volid = $rootinfo->{volume};
+    lock_config($vmid, $updatefn);
 
-    eval {
-       PVE::Storage::volume_snapshot_delete($storecfg, $volid, $snapname);
+    # now remove vmstate file
+    # never set for LXC!
+    my $storecfg = PVE::Storage::config();
+
+    if ($snap->{vmstate}) {
+       die "implement me - saving vmstate\n";
     };
-    my $err = $@;
 
-    if(!$err || ($err && $force)) {
-       lock_container($vmid, 10, $del_snap);
-       if ($err) {
-           die "Can't delete snapshot: $vmid $snapname $err\n";
+    # now remove all volume snapshots
+    foreach_mountpoint($snap, sub {
+       my ($ms, $mountpoint) = @_;
+
+       return if $snapname eq 'vzdump' && $ms ne 'rootfs' && !$mountpoint->{backup};
+       if (!$drivehash || $drivehash->{$ms}) {
+           eval { PVE::Storage::volume_snapshot_delete($storecfg, $mountpoint->{volume}, $snapname); };
+           if (my $err = $@) {
+               die $err if !$force;
+               warn $err;
+           }
        }
-    }
+
+       # save changes (remove mp from snapshot)
+       lock_config($vmid, $updatefn, $ms) if !$force;
+       push @$unused, $mountpoint->{volume};
+    });
+
+    # now cleanup config
+    $prepare = 0;
+    lock_config($vmid, $updatefn);
 }
 
 sub snapshot_rollback {
     my ($vmid, $snapname) = @_;
 
+    my $prepare = 1;
+
     my $storecfg = PVE::Storage::config();
 
     my $conf = load_config($vmid);
 
-    die "you can't rollback if vm is a template\n" if is_template($conf);
+    my $get_snapshot_config = sub {
 
-    my $snap = $conf->{snapshots}->{$snapname};
+       die "you can't rollback if vm is a template\n" if is_template($conf);
 
-    die "snapshot '$snapname' does not exist\n" if !defined($snap);
+       my $res = $conf->{snapshots}->{$snapname};
 
-    my $rootfs = $snap->{rootfs};
-    my $rootinfo = parse_ct_rootfs($rootfs);
-    my $volid = $rootinfo->{volume};
+       die "snapshot '$snapname' does not exist\n" if !defined($res);
+
+       return $res;
+    };
 
-    PVE::Storage::volume_rollback_is_possible($storecfg, $volid, $snapname);
+    my $snap = &$get_snapshot_config();
+
+    foreach_mountpoint($snap, sub {
+       my ($ms, $mountpoint) = @_;
+
+       PVE::Storage::volume_rollback_is_possible($storecfg, $mountpoint->{volume}, $snapname);
+    });
 
     my $updatefn = sub {
 
-       die "unable to rollback to incomplete snapshot (snapstate = $snap->{snapstate})\n" 
-           if $snap->{snapstate};
+       $conf = load_config($vmid);
 
-       check_lock($conf);
+       $snap = &$get_snapshot_config();
 
-       system("lxc-stop -n $vmid --kill") if check_running($vmid);
+       die "unable to rollback to incomplete snapshot (snapstate = $snap->{snapstate})\n"
+           if $snap->{snapstate};
+
+       if ($prepare) {
+           check_lock($conf);
+           PVE::Tools::run_command(['/usr/bin/lxc-stop', '-n', $vmid, '--kill'])
+               if check_running($vmid);
+       }
 
        die "unable to rollback vm $vmid: vm is running\n"
            if check_running($vmid);
 
-       $conf->{lock} = 'rollback';
+       if ($prepare) {
+           $conf->{lock} = 'rollback';
+       } else {
+           die "got wrong lock\n" if !($conf->{lock} && $conf->{lock} eq 'rollback');
+           delete $conf->{lock};
+       }
 
        my $forcemachine;
 
-       # copy snapshot config to current config
-
-       my $tmp_conf = $conf;
-       &$snapshot_copy_config($tmp_conf->{snapshots}->{$snapname}, $conf);
-       $conf->{snapshots} = $tmp_conf->{snapshots};
-       delete $conf->{snaptime};
-       delete $conf->{snapname};
-       $conf->{parent} = $snapname;
+       if (!$prepare) {
+           # copy snapshot config to current config
+           $conf = &$snapshot_apply_config($conf, $snap);
+           $conf->{parent} = $snapname;
+       }
 
        write_config($vmid, $conf);
-    };
 
-    my $unlockfn = sub {
-       delete $conf->{lock};
-       write_config($vmid, $conf);
+       if (!$prepare && $snap->{vmstate}) {
+           die "implement me - save vmstate\n";
+       }
     };
 
-    lock_container($vmid, 10, $updatefn);
+    lock_config($vmid, $updatefn);
 
-    PVE::Storage::volume_snapshot_rollback($storecfg, $volid, $snapname);
+    foreach_mountpoint($snap, sub {
+       my ($ms, $mountpoint) = @_;
+
+       PVE::Storage::volume_snapshot_rollback($storecfg, $mountpoint->{volume}, $snapname);
+    });
 
-    lock_container($vmid, 5, $unlockfn);
+    $prepare = 0;
+    lock_config($vmid, $updatefn);
 }
 
 sub template_create {
@@ -2107,16 +2265,20 @@ sub foreach_mountpoint_reverse {
 }
 
 sub check_ct_modify_config_perm {
-    my ($rpcenv, $authuser, $vmid, $pool, $key_list) = @_;
+    my ($rpcenv, $authuser, $vmid, $pool, $newconf, $delete) = @_;
 
-    return 1 if $authuser ne 'root@pam';
-
-    foreach my $opt (@$key_list) {
+    return 1 if $authuser eq 'root@pam';
 
+    my $check = sub {
+       my ($opt, $delete) = @_;
        if ($opt eq 'cpus' || $opt eq 'cpuunits' || $opt eq 'cpulimit') {
            $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.CPU']);
        } elsif ($opt eq 'rootfs' || $opt =~ /^mp\d+$/) {
            $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Disk']);
+           return if $delete;
+           my $data = $opt eq 'rootfs' ? parse_ct_rootfs($newconf->{$opt})
+                                       : parse_ct_mountpoint($newconf->{$opt});
+           raise_perm_exc("mountpoint type $data->{type}") if $data->{type} ne 'volume';
        } elsif ($opt eq 'memory' || $opt eq 'swap') {
            $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Memory']);
        } elsif ($opt =~ m/^net\d+$/ || $opt eq 'nameserver' ||
@@ -2125,6 +2287,13 @@ sub check_ct_modify_config_perm {
        } else {
            $rpcenv->check_vm_perm($authuser, $vmid, $pool, ['VM.Config.Options']);
        }
+    };
+
+    foreach my $opt (keys %$newconf) {
+       &$check($opt, 0);
+    }
+    foreach my $opt (@$delete) {
+       &$check($opt, 1);
     }
 
     return 1;
@@ -2238,6 +2407,20 @@ sub run_with_loopdev {
     return $device;
 }
 
+sub bindmount {
+    my ($dir, $dest, $ro, @extra_opts) = @_;
+    PVE::Tools::run_command(['mount', '-o', 'bind', @extra_opts, $dir, $dest]);
+    if ($ro) {
+       eval { PVE::Tools::run_command(['mount', '-o', 'bind,remount,ro', $dest]); };
+       if (my $err = $@) {
+           warn "bindmount error\n";
+           # don't leave writable bind-mounts behind...
+           PVE::Tools::run_command(['umount', $dest]);
+           die $err;
+       }
+    }
+}
+
 # use $rootdir = undef to just return the corresponding mount path
 sub mountpoint_mount {
     my ($mountpoint, $rootdir, $storage_cfg, $snapname) = @_;
@@ -2268,10 +2451,7 @@ sub mountpoint_mount {
     if (defined($mountpoint->{acl})) {
        $optstring .= ($mountpoint->{acl} ? 'acl' : 'noacl');
     }
-    if ($mountpoint->{ro}) {
-       $optstring .= ',' if $optstring;
-       $optstring .= 'ro';
-    }
+    my $readonly = $mountpoint->{ro};
 
     my @extra_opts = ('-o', $optstring);
 
@@ -2296,10 +2476,7 @@ sub mountpoint_mount {
                        die "cannot mount subvol snapshots for storage type '$scfg->{type}'\n";
                    }
                } else {
-                   if ($mountpoint->{ro}) {
-                       die "read-only bind mounts not supported\n";
-                   }
-                   PVE::Tools::run_command(['mount', '-o', 'bind', @extra_opts, $path, $mount_path]);
+                   bindmount($path, $mount_path, $readonly, @extra_opts);
                    warn "cannot enable quota control for bind mounted subvolumes\n" if $quota;
                }
            }
@@ -2316,6 +2493,7 @@ sub mountpoint_mount {
                        if ($quota) {
                            push @extra_opts, '-o', 'usrjquota=aquota.user,grpjquota=aquota.group,jqfmt=vfsv0';
                        }
+                       push @extra_opts, '-o', 'ro' if $readonly;
                        PVE::Tools::run_command(['mount', @extra_opts, $path, $mount_path]);
                    }
                }
@@ -2336,18 +2514,13 @@ sub mountpoint_mount {
            die "unsupported image format '$format'\n";
        }
     } elsif ($type eq 'device') {
+                       push @extra_opts, '-o', 'ro' if $readonly;
        PVE::Tools::run_command(['mount', @extra_opts, $volid, $mount_path]) if $mount_path;
        return wantarray ? ($volid, 0, $volid) : $volid;
     } elsif ($type eq 'bind') {
-       if ($mountpoint->{ro}) {
-           die "read-only bind mounts not supported\n";
-           # Theoretically we'd have to execute both:
-           # mount -o bind $a $b
-           # mount -o bind,remount,ro $a $b
-       }
        die "directory '$volid' does not exist\n" if ! -d $volid;
        &$check_mount_path($volid);
-       PVE::Tools::run_command(['mount', '-o', 'bind', @extra_opts, $volid, $mount_path]) if $mount_path;
+       bindmount($volid, $mount_path, $readonly, @extra_opts) if $mount_path;
        warn "cannot enable quota control for bind mounts\n" if $quota;
        return wantarray ? ($volid, 0, undef) : $volid;
     }