Re: [PATCH v2 0/8] Introduce network backed NVRAM
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(a)nutanix.com> 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_a...
)
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
>