On April 29, 2020 11:58 am, Fabian Ebner wrote: > The update_pct_config call leads to a write_config call and so the > configuration file was created before it was intended to be created. > > When the CFS is updated in between the write_config call and the > PVE::Cluster::check_vmid_unused call in create_and_lock_config, > the container file would already exist and so creation would > fail after writing out a basically empty config. > (Meaning this is necessary for the proposed "update CFS after > obtaining a configuration file lock" changes) > > Even worse, a race was possible for two containers created with the > same ID at the same time: > Assuming the initial PVE::Cluster::check_vmid_unused check in the > parameter verification passes for both create_vm calls, the later one > would potentially overwrite the earlier configuration file with its > update_pct_config call. > > Additionally, the file read for $old_config was always the one written > by update_pct_config. Meaning that for a create_vm call with force=1, > already existing old volumes were not removed. > > Signed-off-by: Fabian Ebner <f.eb...@proxmox.com>
hmm, update_pct_config there was not intended to write out the config, but just to fill the $conf hash. three alternative approaches that would return to the original semantics: 1 skip hotplug/apply pending if pending section is empty (should always be the case in this code path) 2 add explicit parameter to skip 3 drop the hotplug/apply pending calls in update_pct_config, add them to the update_vm API endpoint instead. I'd prefer 3 - 1 - 2 in that order ;) 3 would be called with the same semantics in both call sites (create_vm and update_vm): here are add/delete/revert parameters, here's a conf hash; merge those operating solely on the conf hash. > --- > > Note that this should be applied before [0] to avoid temporary > (further ;)) breakage of container creation. > > [0]: https://pve.proxmox.com/pipermail/pve-devel/2020-April/043155.html > > src/PVE/API2/LXC.pm | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm > index 148ba6a..46d2fd7 100644 > --- a/src/PVE/API2/LXC.pm > +++ b/src/PVE/API2/LXC.pm > @@ -334,10 +334,6 @@ __PACKAGE__->register_method({ > # check/activate default storage > &$check_and_activate_storage($storage) if !defined($mp_param->{rootfs}); > > - PVE::LXC::Config->update_pct_config($vmid, $conf, 0, $no_disk_param); > - > - $conf->{unprivileged} = 1 if $unprivileged; > - > my $emsg = $restore ? "unable to restore CT $vmid -" : "unable to > create CT $vmid -"; > > eval { PVE::LXC::Config->create_and_lock_config($vmid, $force) }; > @@ -346,8 +342,11 @@ __PACKAGE__->register_method({ > my $code = sub { > my $old_conf = PVE::LXC::Config->load_config($vmid); > my $was_template; > - > my $vollist = []; > + > + PVE::LXC::Config->update_pct_config($vmid, $conf, 0, > $no_disk_param); > + $conf->{unprivileged} = 1 if $unprivileged; > + > eval { > my $orig_mp_param; # only used if $restore > if ($restore) { > -- > 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
