On Fri, Apr 8, 2022 at 11:19 PM Rohit Kumar <rohit.kumar3@xxxxxxxxxxx> wrote: > > Libvirt domain XML currently allows only local filepaths > that can be used to specify a NVRAM disk. > Since, VMs can migrate across hypervisor hosts, it should be > possible to allocate NVRAM disks on network storage for > uninterrupted access. > > This series extends the NVRAM element to support hosting over > network-backed disks, for high availability. > It achieves this by embedding virStorageSource pointer for > nvram into _virDomainLoaderDef. > > It introduces a 'type' attribute for NVRAM element to > specify 'file' vs 'network' backed NVRAM. > > Changes v1->v2: > - Split the patch into smaller patches > - Added unit test > - Updated the doc > - Addressed Peter's comment on v1 (https://listman.redhat.com/archives/libvir-list/2022-March/229684.html) Ok so without going deeper into the actual change, following are some quick comments based on some of my own experience of introducing new conf options in libvirt: (a) Please update NEWS.rst t to document the new xml attribute support for the next release. This should be the last patch in the series preferrably. (b) Please put patch #2 and #5 together. Also please prefix the first line with "conf:" I think patch #4 should also go together but I will let others comment. (c) It's better that the unit tests (patches #7 and #8) go along with the changes in the same patch. Then when cherry picking the unit tests will be picked along with the change itself. (d) also I have commented separately, your validation patch needs additional unit tests to check the validation actually works. > > Rohit Kumar (8): > Make NVRAM a virStorageSource type. > Add support to parse/format virStorageSource type NVRAM > Validate remote store NVRAM > Cleanup diskSourceNetwork and diskSourceFile schema > Update XML schema to support network backed NVRAM > Update NVRAM documentation > Add unit test for network backed NVRAM > Add unit test to support new 'file' type NVRAM > > docs/formatdomain.rst | 43 +++++++-- > src/conf/domain_conf.c | 88 ++++++++++++++++--- > src/conf/domain_conf.h | 2 +- > src/conf/schemas/domaincommon.rng | 80 +++++++++++------ > src/qemu/qemu_cgroup.c | 3 +- > src/qemu/qemu_command.c | 2 +- > src/qemu/qemu_domain.c | 14 +-- > src/qemu/qemu_driver.c | 5 +- > src/qemu/qemu_firmware.c | 23 +++-- > src/qemu/qemu_namespace.c | 5 +- > src/qemu/qemu_process.c | 5 +- > src/qemu/qemu_validate.c | 22 +++++ > src/security/security_dac.c | 6 +- > src/security/security_selinux.c | 6 +- > src/security/virt-aa-helper.c | 5 +- > src/vbox/vbox_common.c | 2 +- > .../bios-nvram-file.x86_64-latest.args | 37 ++++++++ > tests/qemuxml2argvdata/bios-nvram-file.xml | 23 +++++ > .../bios-nvram-network.x86_64-latest.args | 37 ++++++++ > tests/qemuxml2argvdata/bios-nvram-network.xml | 25 ++++++ > tests/qemuxml2argvtest.c | 2 + > 21 files changed, 360 insertions(+), 75 deletions(-) > create mode 100644 tests/qemuxml2argvdata/bios-nvram-file.x86_64-latest.args > create mode 100644 tests/qemuxml2argvdata/bios-nvram-file.xml > create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.x86_64-latest.args > create mode 100644 tests/qemuxml2argvdata/bios-nvram-network.xml > > -- > 2.25.1 >