[pve-devel] [PATCH container] fix #2820: block adding new volume with same id if it's pending delete

[Oguz Bektas]

fabian's variant can be done like this:
-------------------------------------------------------------------------------------------

diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index 0a28380..ba5e548 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -1248,6 +1248,9 @@ sub vmconfig_hotplug_pending {
                    die "skip\n";
                }

+               if (exists($conf->{$opt})) {
+                   die "skip\n";
+               }
                $class->apply_pending_mountpoint($vmid, $conf, $opt, $storecfg, 1);
                # apply_pending_mountpoint modifies the value if it creates a new disk
                $value = $conf->{pending}->{$opt};

-------------------------------------------------------------------------------------------

we just check if the mpX is already in the config, if yes then the hotplug is skipped, adding it as a pending change for the next reboot.
the "replaced" disk becomes unused.



On Wed, Jul 01, 2020 at 02:50:06PM +0200, Thomas Lamprecht wrote:
> On 01.07.20 14:43, Fabian Grünbichler wrote:
> > On July 1, 2020 2:05 pm, Thomas Lamprecht wrote:
> >> On 01.07.20 09:11, Fabian Grünbichler wrote:
> >>> - we can actually just put the new mpX into the pending queue, and 
> >>>   remove the entry from the pending deletion queue? (it's hotplugging 
> >>>   that is the problem, not queuing the pending change)
> >>
> >> Even if we could I'm not sure I want to be able to add a new mpX as pending
> >> if the old is still pending its deletion. But, tbh, I did not looked at details
> >> so I may missing something..
> > 
> > well, the sequence is
> > 
> > - delete mp0 (queued)
> > - set a new mp0 (queued)
> > 
> > just like a general
> > 
> > - delete foo  (queued)
> > - set foo (queued)
> > 
> > where the set removes the queued deletion. in the case of mp, applying 
> > that pending change should then add the old volume ID as unused, but 
> > that IMHO does not change the semantics of '(queuing a) set overrides 
> > earlier queued delete'.
> 
> IMO the set mpX isn't your general option setting, and I'd just not allow
> re-setting it with a delete still pending, to dangerous IMO.
> Maybe better make it clear for the user that they either need to apply the
> pending change (e.g., CT reboot), revert it or just use another mpX id.


if this is too dangerous, then i'll instead make a v3, changing the match logic to work with the parse_pending_delete helper.

which is better?

> 
> > 
> > but this is broken for regular hotplug without deletion as well, setting 
> > mpX with a new volume ID if the slot is already used does not queue it 
> > as pending change, but
> > - mounts the new volume ID in addition to the old one
> > - adds the old volume ID as unused, even though it is still mounted in 
> >   the container
> 
> gosh.. yeah that needs to fail too.
> 
> > 
> > so this is broken in more ways than just what I initially found..
> > 
>