On Sat, Sep 07, 2024 at 17:15:15 +0300, Nikolai Barybin via Devel wrote:
> Hello everyone!
>
> With help of Peter's review and after researching Cole's patches I've
> come up with the second version.
>
> Changes since last revision:
>
> - properly taken in account (while probing disk chain) usecase when we have
> data-file as part of some backing image
>
> - added proper integration with security drivers instead of call to
> chown
>
> - data-file is added to qemu cmdline as a reference to blockdev
>
> - added XML formatiing and parsing
>
> - added basic tests to qemublocktest
>
> Nikolai Barybin (13):
> conf: add data-file feature and related fields to virStorageSource
> storage file: add getDataFile function to FileTypeInfo
> storage file: add qcow2 data-file path parsing from header
> storage file: fill in src->dataFileStore during file probe
> security: DAC: handle qcow2 data-file on image label set/restore
> security: selinux: handle qcow2 data-file on image label set/restore
> security: apparmor: handle qcow2 data-file
> qemu: put data-file path to VM's cgroup and namespace
> qemu: factor out qemuDomainPrepareStorageSource()
> qemu: enable basic qcow2 data-file feature support
> conf: schemas: add data-file store to domain rng schema
> conf: implement XML parsing/formatingo for dataFileStore
> tests: add qcow2 data-file basic tests to qemublocktest
>
> src/conf/domain_conf.c | 98 +++++++++++++++++++
> src/conf/domain_conf.h | 13 +++
> src/conf/schemas/domaincommon.rng | 15 +++
> src/conf/storage_source_conf.c | 11 +++
> src/conf/storage_source_conf.h | 5 +
> src/qemu/qemu_block.c | 7 ++
> src/qemu/qemu_cgroup.c | 4 +
> src/qemu/qemu_command.c | 5 +
> src/qemu/qemu_domain.c | 50 +++++++---
> src/qemu/qemu_namespace.c | 5 +
> src/security/security_dac.c | 26 ++++-
> src/security/security_selinux.c | 20 +++-
> src/security/virt-aa-helper.c | 4 +
> src/storage_file/storage_file_probe.c | 85 ++++++++++++----
> src/storage_file/storage_source.c | 28 ++++++
> src/storage_file/storage_source.h | 3 +
> tests/qemublocktest.c | 78 +++++++++------
> ...backing-with-data-file-noopts-srconly.json | 27 +++++
> ...e-qcow2-backing-with-data-file-noopts.json | 41 ++++++++
> ...le-qcow2-backing-with-data-file-noopts.xml | 35 +++++++
> .../file-qcow2-data-file-noopts-srconly.json | 18 ++++
> .../xml2json/file-qcow2-data-file-noopts.json | 27 +++++
> .../xml2json/file-qcow2-data-file-noopts.xml | 24 +++++
> 23 files changed, 558 insertions(+), 71 deletions(-)
> create mode 100644 tests/qemublocktestdata/xml2json/file-qcow2-backing-with-data-file-noopts-srconly.json
> create mode 100644 tests/qemublocktestdata/xml2json/file-qcow2-backing-with-data-file-noopts.json
> create mode 100644 tests/qemublocktestdata/xml2json/file-qcow2-backing-with-data-file-noopts.xml
> create mode 100644 tests/qemublocktestdata/xml2json/file-qcow2-data-file-noopts-srconly.json
> create mode 100644 tests/qemublocktestdata/xml2json/file-qcow2-data-file-noopts.json
> create mode 100644 tests/qemublocktestdata/xml2json/file-qcow2-data-file-noopts.xml
Couple notes before I get to a proper review (I have some other things I
need to finish out). This is what I see when looking at the diffstat and
commit messages:
- 'syntax-check' fails on last patch
https://www.libvirt.org/hacking.html#preparing-patches
- commit messages are missing in some patches
- make sure to describe the rationale/justification rather than just
summarizing the patch
- preferrably move XML input/output patch before adding the detection
- 'qemuxmlconftest' test case missing (with explicitly defined backing
store as the detection from files is mocked out)
- make sure to add multiple disks specifying the data file as a
'file', 'block' and 'network' backed storage
- 'virstoragetest' test case for the qcow2 metadata changes
- you'll need to add a minimalistic test QCOW2 file with data file
- add cases for lookup and modify the test to output the data file
path if present
