On February 3, 2022 10:31 am, Fabian Ebner wrote: > Am 31.01.22 um 13:34 schrieb Fabian Grünbichler: >> On January 27, 2022 3:01 pm, Fabian Ebner wrote: >>> using the familiar early+repeated checks pattern from other API calls. >>> Only intended functional changes are with regard to locking/forking. >> >> two questions: >> - the FW config cloning happens inside the worker now, while it was >> previously before forking the worker (LGTM, but might be called out >> explicitly if intentional ;)) > > Honestly, I didn't think too much about it, so thanks for pointing that > out! But thinking about it now, I also don't see an obvious issue with > it and IMHO it feels more natural to be part of the worker since it > takes the firewall config lock and the cleanup also happens inside the > worker. > >> - there are some checks at the start of the endpoint (checking >> storage/target), which are not repeated after the fork+lock - while >> unlikely, our view of storage.cfg could change in-between (lock guest >> config -> cfs_update). should those be moved in the check sub (or into >> the check_storage_access_clone helper)? >> > > Yes, for better consistency that should be done. Either way is fine with > me. Should I send a v2 or are you going to do a follow-up?
I'll do a follow-up! > >> rest of the series LGTM _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
