On October 3, 2019 4:32 pm, Oguz Bektas wrote:
> On Wed, Oct 02, 2019 at 01:52:58PM +0200, Fabian Grünbichler wrote:
>> 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 ;)
>
> i actually wanted to move even more. in qemu api it's like:
>
> -----------------------------------
>
> -----------------------------------
> __PACKAGE__->register_method({
> name => 'update_vm_async',
> path => '{vmid}/config',
> method => 'POST',
> ...
>
> returns => {
> type => 'string',
> optional => 1,
> },
> code => $update_vm_api,
> });
> -----------------------------------
>
> and
>
> -----------------------------------
> __PACKAGE__->register_method({
> name => 'update_vm',
> path => '{vmid}/config',
> method => 'PUT',
> protected => 1,
> proxyto => 'node',
> description => "Set virtual machine options (synchrounous API) - You
> should consider using the POST method instead for any actions involving
> hotplug or storage allocation.",
> ...
> returns => { type => 'null' },
> code => sub {
> my ($param) = @_;
> &$update_vm_api($param, 1);
> return undef;
> }
> });
> -----------------------------------
>
> which i find more readable than the current situation in container code, and
> the checks are in the $update_vm_api sub.
yes, but in qemu-server ALL of that is in the API file, not in
QemuServer.pm ;) you can try to see how moving update_pct_config to the
API feels, all callers are in the API anyway.
>
> now i don't know which is better in terms of implementation, so if you
> believe that the checks should stay in api, then we can do it like that.
>
>>
>> >
>> > 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
>>
>
> _______________________________________________
> 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
