[pve-devel] [PATCH v2 00/11] Refactor QemuServer to avoid dependency cycles

[Thomas Lamprecht]

On 10/30/19 11:49 AM, Fabian Grünbichler wrote:
> On October 28, 2019 12:59 pm, Stefan Reiter wrote:
>> First 3 patches are independant refactorings around get_host_arch.
>>
>> Rest of the series refactors QemuServer and creates three new packages:
>> * 'PVE::QemuSchema' for schema related code and common directory creation
>> * 'PVE::QMP' for higher-level QMP functions
>> * 'PVE::QemuServer::Machine' for QEMU machine-type related helpers
>>
>> This refactoring came along because qemu_machine_feature_enabled needs to be
>> used in 'PVE::QemuServer::CPUConfig', a new package that will be introduced with
>> my custom CPU series [0]. This would currently require dependency cycles, but by
>> extracting the code in this series and splitting it up into multiple helper
>> modules, this can be avoided.
>>
>> Care was taken not to introduce new dependecy cycles, though this required to
>> move the 'check_running' function to QemuConfig.pm, where it doesn't *quite* fit
>> IMO, but I also didn't want to create a new module just for this one function.
>> Open for ideas ofc.
> 
> thanks for this big chunk of work! some comments on individual patches, 
> and one high-level remark that we already discussed off-list:
> 
> I'd use this opportunity to rename PVE/QemuServer to PVE/Qemu, and move 
> all/most of the split out files into PVE/Qemu as well.
We talked about this off-list, and as said there this is something I
thought of too, so ACK. But, I'd rather not do it over the mailinglist
(@Stefan), but that's something I, or you (@Fabian), can do directly in
git and push it out. I see it unrelated to the refactoring also, so not
something you should pay attention to in a v2, Stefan.

> 
> we could think about moving QemuConfig and QemuMigrate there as well 
> (like we have in pve-container), but that would mean touching even more 
> files, including pve-firewall (which uses PVE::QemuConfig).
> 

If we start doing this then fully, a little headache more, or less, is
rather irrelevant - IMO.