On 7/4/24 12:49, Fabian Grünbichler wrote: > so if I understand this correctly, it should handle the following case: > > proxmox-firewall runs and sets up NFT rules > user disables NFT > pve-firewall runs and sets up legacy rules and force disable file > proxmox-firewall runs and disables NFT rules > > as opposed to the following sequence > > proxmox-firewall runs and sets up NFT rules > user disables NFT > proxmox-firewall runs and disables NFT rules > pve-firewall runs and sets up legacy rules and force disable file > > which is already handled..
correct > I don't see why the first cast should "almost never happen", just because the > loops have a different period - it all comes down to alignment of the periods > and timing of the user action? > > in other words, you have a sequence > > 0: N > 5: N > 5+X: L > 10: N > 15: N > 15+X: L > 20: N > 25: N > 25+X: L > > where the gap between N and N is 5 seconds, and the gap between N and L and L > and N together is also 5 seconds. on average (assuming random alignment of the > periods), there's an X=2.5s window (out of 10) that the user action must hit > to > trigger the issue (in the gap between L and N, since X can be between 0 and > 5s)? > > FWIW, the change itself looks good to me, but the commit message might need > some adaptation ;) You're correct, the reasoning in the commit message is wrong or at least badly worded and I'll try to improve upon that in a v2. I guess it might even be unnecessary to mention how often / rarely this can happen and focus more on describing the race condition itself. _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
