Thanks for taking the time to review this, Ani!
On 09/04/22 8:22 pm, Ani Sinha wrote:
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://urldefense.proofpoint.com/v2/url?u=https-3A__listman.redhat.com_archives_libvir-2Dlist_2022-2DMarch_229684.html&d=DwIBaQ&c=s883GpUCOChKOHiocYtGcg&r=ABSkr7gy7ZTfApFfI-Xxt1gZNtsDDiXoXOXc0OrkyFs&m=2qaiZSbuvF3-NV3omL5Oy7HR2A7UtREZi77Fq1EtOI0rL4dFj8bT74YoovtK373G&s=UFWNI0THYsFToxpv6P_ZHpTzpIB45eECG3Ltl8rRYHc&e= )
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.
Yes, thanks. I will update it.
(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.
On patch v1, Peter suggested to saperate the cleanups in a different patch.
Sure, putting #2 and #5 would be good idea.
(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.
Yes, this seems logical. I would also prefer to wait for Peter's review
before sending out the next patch.
(d) also I have commented separately, your validation patch needs
additional unit tests to check the validation actually works.
Ack., thanks.
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
