Re: [pve-devel] [PATCH qemu-server 2/4] api: clone: fork before locking

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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