[pve-devel] [PATCH v3 0/8] Refactor QemuServer to avoid dependency cycles

[Fabian Grünbichler]

On November 4, 2019 2:57 pm, Stefan Reiter wrote:
> This series refactors QemuServer and creates three new packages:
> * 'PVE::QemuServer::Helpers' for general purpose helpers
> * 'PVE::QemuServer::Monitor' 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.
> 
> New functions are created as PVE::QemuServer, not PVE::Qemu, to be consistent
> for now.

some comments on indivual patches, looking mostly good or even great!

it's always good to note inter-package breakage/dependencies, such as in 
this case:

qemu-server versioned breaks on old pve-ha-manager & pve-manager
pve-manager & pve-ha-manager versioned dependency on new qemu-server

either in the cover-letter, or on the individual patches (IIRC Thomas 
prefers the latter). in this case breakage would be obvious, but that is 
not always the case.

> v3:
> * QMP -> QemuServer::Monitor
> * QemuSchema -> QemuServer::Helpers (more or less)
> * split check_running (but keep old version to not change all callers)
> * split qemu_machine_feature_enabled
> * include "check_cmdline -> parse_cmdline" patch in series (needed for later
>   live migration with custom CPU types anyway)
> * redo patches to pve-manager/pve-ha-manager to accomodate changes in v3
> 
> v2:
> * Actually test changes correctly - sorry
> * Fix a few package 'use's I missed to move to new packages
> * Fix tests for pve-manager
> * Fix missing '=' in pve-container
> 
> [0] https://pve.proxmox.com/pipermail/pve-devel/2019-October/039608.html
> 
> 
> qemu-server: Stefan Reiter (6):
>   refactor: create QemuServer::Helpers and move file/dir code
>   Change check_cmdline to parse_cmdline
>   refactor: split check_running into _exists_ and _running_
>   refactor: create QemuServer::Monitor for high-level QMP access
>   refactor: extract QEMU machine related helpers to package
>   refactor: split qemu_machine_feature_enabled
> 
>  PVE/API2/Qemu.pm                     |  15 +-
>  PVE/API2/Qemu/Agent.pm               |   7 +-
>  PVE/CLI/qm.pm                        |  15 +-
>  PVE/QMPClient.pm                     |   5 +-
>  PVE/QemuConfig.pm                    |  42 ++-
>  PVE/QemuMigrate.pm                   |  24 +-
>  PVE/QemuServer.pm                    | 421 +++++++--------------------
>  PVE/QemuServer/Agent.pm              |   3 +-
>  PVE/QemuServer/Helpers.pm            | 138 +++++++++
>  PVE/QemuServer/Machine.pm            |  74 +++++
>  PVE/QemuServer/Makefile              |   3 +
>  PVE/QemuServer/Memory.pm             |   9 +-
>  PVE/VZDump/QemuServer.pm             |  16 +-
>  test/cfg2cmd/pinned-version.conf     |  15 +
>  test/cfg2cmd/pinned-version.conf.cmd |  30 ++
>  test/snapshot-test.pm                |  31 +-
>  16 files changed, 465 insertions(+), 383 deletions(-)
>  create mode 100644 PVE/QemuServer/Helpers.pm
>  create mode 100644 PVE/QemuServer/Machine.pm
>  create mode 100644 test/cfg2cmd/pinned-version.conf
>  create mode 100644 test/cfg2cmd/pinned-version.conf.cmd
> 
> ha-manager: Stefan Reiter (1):
>   refactor: vm_qmp_command was moved to PVE::QemuServer::Monitor
> 
>  src/PVE/HA/Resources/PVEVM.pm | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> 
> qemu-server: Stefan Reiter (6):
>   refactor: create QemuServer::Helpers and move file/dir code
>   Change check_cmdline to parse_cmdline
>   refactor: split check_running into _exists_ and _running_
>   refactor: create QemuServer::Monitor for high-level QMP access
>   refactor: extract QEMU machine related helpers to package
>   refactor: split qemu_machine_feature_enabled
> 
>  PVE/API2/Qemu.pm                     |  15 +-
>  PVE/API2/Qemu/Agent.pm               |   7 +-
>  PVE/CLI/qm.pm                        |  16 +-
>  PVE/QMPClient.pm                     |   5 +-
>  PVE/QemuConfig.pm                    |  42 ++-
>  PVE/QemuMigrate.pm                   |  24 +-
>  PVE/QemuServer.pm                    | 421 +++++++--------------------
>  PVE/QemuServer/Agent.pm              |   3 +-
>  PVE/QemuServer/Helpers.pm            | 138 +++++++++
>  PVE/QemuServer/Machine.pm            |  74 +++++
>  PVE/QemuServer/Makefile              |   3 +
>  PVE/QemuServer/Memory.pm             |   9 +-
>  PVE/VZDump/QemuServer.pm             |  16 +-
>  test/cfg2cmd/pinned-version.conf     |  15 +
>  test/cfg2cmd/pinned-version.conf.cmd |  30 ++
>  test/snapshot-test.pm                |  31 +-
>  16 files changed, 466 insertions(+), 383 deletions(-)
>  create mode 100644 PVE/QemuServer/Helpers.pm
>  create mode 100644 PVE/QemuServer/Machine.pm
>  create mode 100644 test/cfg2cmd/pinned-version.conf
>  create mode 100644 test/cfg2cmd/pinned-version.conf.cmd
> 
> ha-manager: Stefan Reiter (1):
>   refactor: vm_qmp_command was moved to PVE::QemuServer::Monitor
> 
>  src/PVE/HA/Resources/PVEVM.pm | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> 
> qemu-server: Stefan Reiter (6):
>   refactor: create QemuServer::Helpers and move file/dir code
>   Change check_cmdline to parse_cmdline
>   refactor: split check_running into _exists_ and _running_
>   refactor: create QemuServer::Monitor for high-level QMP access
>   refactor: extract QEMU machine related helpers to package
>   refactor: split qemu_machine_feature_enabled
> 
>  PVE/API2/Qemu.pm                     |  15 +-
>  PVE/API2/Qemu/Agent.pm               |   7 +-
>  PVE/CLI/qm.pm                        |  16 +-
>  PVE/QMPClient.pm                     |   5 +-
>  PVE/QemuConfig.pm                    |  42 ++-
>  PVE/QemuMigrate.pm                   |  24 +-
>  PVE/QemuServer.pm                    | 421 +++++++--------------------
>  PVE/QemuServer/Agent.pm              |   3 +-
>  PVE/QemuServer/Helpers.pm            | 138 +++++++++
>  PVE/QemuServer/Machine.pm            |  73 +++++
>  PVE/QemuServer/Makefile              |   3 +
>  PVE/QemuServer/Memory.pm             |   9 +-
>  PVE/VZDump/QemuServer.pm             |  16 +-
>  test/cfg2cmd/pinned-version.conf     |  15 +
>  test/cfg2cmd/pinned-version.conf.cmd |  30 ++
>  test/snapshot-test.pm                |  31 +-
>  16 files changed, 465 insertions(+), 383 deletions(-)
>  create mode 100644 PVE/QemuServer/Helpers.pm
>  create mode 100644 PVE/QemuServer/Machine.pm
>  create mode 100644 test/cfg2cmd/pinned-version.conf
>  create mode 100644 test/cfg2cmd/pinned-version.conf.cmd
> 
> ha-manager: Stefan Reiter (1):
>   refactor: vm_qmp_command was moved to PVE::QemuServer::Monitor
> 
>  src/PVE/HA/Resources/PVEVM.pm | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> manager: Stefan Reiter (1):
>   refactor: vm_mon_cmd is now Monitor::mon_cmd
> 
>  PVE/Service/pvestatd.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> -- 
> 2.20.1
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>