On September 30, 2019 2:44 pm, Oguz Bektas wrote:
> we don't need to extract 'delete' here, instead we pass it all as $param
> and extract 'delete', 'revert' and most other things in
> update_pct_config
I already asked that in v1 - wouldn't it make sense to keep the
parameter checks in the API call, and pass the already "verified"
$delete and $revert to update_pct_config? this would avoid the need of
adding new module dependencies from PVE::LXC::Config to
PVE::RPCEnvironment and PVE::Exception in #17..
if you see a good reason for moving all that to update_pct_config,
please say so (either in the changelog of the patch, or as reply to the
original review) and don't silently ignore open questions ;)
>
> Signed-off-by: Oguz Bektas <o.bek...@proxmox.com>
> ---
> src/PVE/API2/LXC/Config.pm | 55 ++++----------------------------------
> 1 file changed, 5 insertions(+), 50 deletions(-)
>
> diff --git a/src/PVE/API2/LXC/Config.pm b/src/PVE/API2/LXC/Config.pm
> index 2c036f5..46d8e2f 100644
> --- a/src/PVE/API2/LXC/Config.pm
> +++ b/src/PVE/API2/LXC/Config.pm
> @@ -119,58 +119,10 @@ __PACKAGE__->register_method({
> code => sub {
> my ($param) = @_;
>
> - my $rpcenv = PVE::RPCEnvironment::get();
> - my $authuser = $rpcenv->get_user();
> -
> my $node = extract_param($param, 'node');
> my $vmid = extract_param($param, 'vmid');
> -
> my $digest = extract_param($param, 'digest');
>
> - die "no options specified\n" if !scalar(keys %$param);
> -
> - my $delete_str = extract_param($param, 'delete');
> - my @delete = PVE::Tools::split_list($delete_str);
> -
> - PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef,
> {}, [@delete]);
> -
> - foreach my $opt (@delete) {
> - raise_param_exc({ delete => "you can't use '-$opt' and -delete
> $opt' at the same time" })
> - if defined($param->{$opt});
> -
> - if (!PVE::LXC::Config->option_exists($opt)) {
> - raise_param_exc({ delete => "unknown option '$opt'" });
> - }
> - }
> -
> - PVE::LXC::check_ct_modify_config_perm($rpcenv, $authuser, $vmid, undef,
> $param, []);
> -
> - my $storage_cfg = cfs_read_file("storage.cfg");
> -
> - my $repl_conf = PVE::ReplicationConfig->new();
> - my $is_replicated = $repl_conf->check_for_existing_jobs($vmid, 1);
> - if ($is_replicated) {
> - PVE::LXC::Config->foreach_mountpoint_full($param, 0, sub {
> - my ($opt, $mountpoint) = @_;
> - my $volid = $mountpoint->{volume};
> - return if !$volid || !($mountpoint->{replicate}//1);
> - if ($mountpoint->{type} eq 'volume') {
> - my ($storeid, $format);
> - if ($volid =~ $PVE::LXC::NEW_DISK_RE) {
> - $storeid = $1;
> - $format = $mountpoint->{format} ||
> PVE::Storage::storage_default_format($storage_cfg, $storeid);
> - } else {
> - ($storeid, undef) =
> PVE::Storage::parse_volume_id($volid, 1);
> - $format = (PVE::Storage::parse_volname($storage_cfg,
> $volid))[6];
> - }
> - return if PVE::Storage::storage_can_replicate($storage_cfg,
> $storeid, $format);
> - my $scfg = PVE::Storage::storage_config($storage_cfg,
> $storeid);
> - return if $scfg->{shared};
> - }
> - die "cannot add non-replicatable volume to a replicated VM\n";
> - });
> - }
> -
> my $code = sub {
>
> my $conf = PVE::LXC::Config->load_config($vmid);
> @@ -180,10 +132,13 @@ __PACKAGE__->register_method({
>
> my $running = PVE::LXC::check_running($vmid);
>
> - PVE::LXC::Config->update_pct_config($vmid, $conf, $running, $param,
> \@delete);
> + die "no options specified\n" if !scalar(keys %$param);
> +
> + PVE::LXC::Config->update_pct_config($vmid, $conf, $running, $param);
> + $conf = PVE::LXC::Config->load_config($vmid);
>
> - PVE::LXC::Config->write_config($vmid, $conf);
> PVE::LXC::update_lxc_config($vmid, $conf);
> +
> };
>
> PVE::LXC::Config->lock_config($vmid, $code);
> --
> 2.20.1
>
> _______________________________________________
> 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
