[PATCH v2 00/13] Implement support for QCOW2 data files
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 -- 2.43.5
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/conf/storage_source_conf.c | 11 +++++++++++ src/conf/storage_source_conf.h | 5 +++++ 2 files changed, 16 insertions(+) diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index 908bc5fab2..5d197ee3ca 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -69,6 +69,7 @@ VIR_ENUM_IMPL(virStorageFileFeature, VIR_STORAGE_FILE_FEATURE_LAST, "lazy_refcounts", "extended_l2", + "data_file", ); @@ -826,6 +827,7 @@ virStorageSourceCopy(const virStorageSource *src, def->relPath = g_strdup(src->relPath); def->backingStoreRaw = g_strdup(src->backingStoreRaw); def->backingStoreRawFormat = src->backingStoreRawFormat; + def->dataFileRaw = g_strdup(src->dataFileRaw); def->snapshot = g_strdup(src->snapshot); def->configFile = g_strdup(src->configFile); def->nodenameformat = g_strdup(src->nodenameformat); @@ -891,6 +893,12 @@ virStorageSourceCopy(const virStorageSource *src, return NULL; } + if (src->dataFileStore) { + if (!(def->dataFileStore = virStorageSourceCopy(src->dataFileStore, + false))) + return NULL; + } + if (src->fdtuple) def->fdtuple = g_object_ref(src->fdtuple); @@ -1171,6 +1179,9 @@ virStorageSourceClear(virStorageSource *def) VIR_FREE(def->nodenamestorage); VIR_FREE(def->nodenameformat); + VIR_FREE(def->dataFileRaw); + g_clear_pointer(&def->dataFileStore, virObjectUnref); + virStorageSourceBackingStoreClear(def); VIR_FREE(def->tlsAlias); diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h index 05b4bda16c..fa27e61204 100644 --- a/src/conf/storage_source_conf.h +++ b/src/conf/storage_source_conf.h @@ -88,6 +88,7 @@ VIR_ENUM_DECL(virStorageFileFormat); typedef enum { VIR_STORAGE_FILE_FEATURE_LAZY_REFCOUNTS = 0, VIR_STORAGE_FILE_FEATURE_EXTENDED_L2, + VIR_STORAGE_FILE_FEATURE_DATA_FILE, VIR_STORAGE_FILE_FEATURE_LAST } virStorageFileFeature; @@ -359,6 +360,9 @@ struct _virStorageSource { /* backing chain of the storage source */ virStorageSource *backingStore; + /* qcow2 data file source */ + virStorageSource *dataFileStore; + /* metadata for storage driver access to remote and local volumes */ void *drv; @@ -369,6 +373,7 @@ struct _virStorageSource { /* Name of the child backing store recorded in metadata of the * current file. */ char *backingStoreRaw; + char *dataFileRaw; virStorageFileFormat backingStoreRawFormat; /* metadata that allows identifying given storage source */ -- 2.43.5
On Sat, Sep 07, 2024 at 17:15:17 +0300, Nikolai Barybin via Devel wrote:
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/conf/storage_source_conf.c | 11 +++++++++++ src/conf/storage_source_conf.h | 5 +++++ 2 files changed, 16 insertions(+)
diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c index 908bc5fab2..5d197ee3ca 100644 --- a/src/conf/storage_source_conf.c +++ b/src/conf/storage_source_conf.c @@ -69,6 +69,7 @@ VIR_ENUM_IMPL(virStorageFileFeature, VIR_STORAGE_FILE_FEATURE_LAST, "lazy_refcounts", "extended_l2", + "data_file", );
[..]
diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h index 05b4bda16c..fa27e61204 100644 --- a/src/conf/storage_source_conf.h +++ b/src/conf/storage_source_conf.h @@ -88,6 +88,7 @@ VIR_ENUM_DECL(virStorageFileFormat); typedef enum { VIR_STORAGE_FILE_FEATURE_LAZY_REFCOUNTS = 0, VIR_STORAGE_FILE_FEATURE_EXTENDED_L2, + VIR_STORAGE_FILE_FEATURE_DATA_FILE,
These hunks seems to belong to a different commit as this has nothing to do with fields in 'struct virStorageSource'. Please move them appropriately. Rest looks good.
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/storage_file/storage_file_probe.c | 34 ++++++++++++++------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index 243927d50a..4792b9fdff 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -93,6 +93,7 @@ struct FileTypeInfo { size_t buf_size); int (*getBackingStore)(char **res, int *format, const char *buf, size_t buf_size); + int (*getDataFile)(char **res, char *buf, int format, size_t buf_size); int (*getFeatures)(virBitmap **features, int format, char *buf, ssize_t len); }; @@ -238,18 +239,18 @@ static struct FileEncryptionInfo const qcow2EncryptionInfo[] = { static struct FileTypeInfo const fileTypeInfo[] = { [VIR_STORAGE_FILE_NONE] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_RAW] = { 0, NULL, LV_LITTLE_ENDIAN, -1, 0, {0}, 0, 0, 0, luksEncryptionInfo, - NULL, NULL, NULL }, + NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_DIR] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_BOCHS] = { /*"Bochs Virtual HD Image", */ /* Untested */ 0, NULL, LV_LITTLE_ENDIAN, 64, 4, {0x20000}, - 32+16+16+4+4+4+4+4, 8, 1, NULL, NULL, NULL, NULL + 32+16+16+4+4+4+4+4, 8, 1, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_CLOOP] = { /* #!/bin/sh @@ -258,7 +259,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { */ /* Untested */ 0, NULL, LV_LITTLE_ENDIAN, -1, 0, {0}, - -1, 0, 0, NULL, NULL, NULL, NULL + -1, 0, 0, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_DMG] = { /* XXX QEMU says there's no magic for dmg, @@ -266,45 +267,45 @@ static struct FileTypeInfo const fileTypeInfo[] = { * would have to match) but then disables that check. */ 0, NULL, 0, -1, 0, {0}, - -1, 0, 0, NULL, NULL, NULL, NULL + -1, 0, 0, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_ISO] = { 32769, "CD001", LV_LITTLE_ENDIAN, -2, 0, {0}, - -1, 0, 0, NULL, NULL, NULL, NULL + -1, 0, 0, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_VPC] = { 0, "conectix", LV_BIG_ENDIAN, 12, 4, {0x10000}, - 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, NULL, NULL, NULL, NULL + 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, NULL, NULL, NULL, NULL, NULL }, /* TODO: add getBackingStore function */ [VIR_STORAGE_FILE_VDI] = { 64, "\x7f\x10\xda\xbe", LV_LITTLE_ENDIAN, 68, 4, {0x00010001}, - 64 + 5 * 4 + 256 + 7 * 4, 8, 1, NULL, NULL, NULL, NULL}, + 64 + 5 * 4 + 256 + 7 * 4, 8, 1, NULL, NULL, NULL, NULL, NULL}, /* Not direct file formats, but used for various drivers */ [VIR_STORAGE_FILE_FAT] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_VHD] = { 0, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL, NULL, NULL }, [VIR_STORAGE_FILE_PLOOP] = { 0, "WithouFreSpacExt", LV_LITTLE_ENDIAN, -2, 0, {0}, PLOOP_IMAGE_SIZE_OFFSET, 8, - PLOOP_SIZE_MULTIPLIER, NULL, NULL, NULL, NULL }, + PLOOP_SIZE_MULTIPLIER, NULL, NULL, NULL, NULL, NULL }, /* All formats with a backing store probe below here */ [VIR_STORAGE_FILE_COW] = { 0, "OOOM", LV_BIG_ENDIAN, 4, 4, {2}, - 4+4+1024+4, 8, 1, NULL, NULL, cowGetBackingStore, NULL + 4+4+1024+4, 8, 1, NULL, NULL, cowGetBackingStore, NULL, NULL }, [VIR_STORAGE_FILE_QCOW] = { 0, "QFI", LV_BIG_ENDIAN, 4, 4, {1}, QCOWX_HDR_IMAGE_SIZE, 8, 1, qcow1EncryptionInfo, - NULL, qcowXGetBackingStore, NULL + NULL, qcowXGetBackingStore, NULL, NULL }, [VIR_STORAGE_FILE_QCOW2] = { 0, "QFI", @@ -313,18 +314,19 @@ static struct FileTypeInfo const fileTypeInfo[] = { qcow2EncryptionInfo, qcow2GetClusterSize, qcowXGetBackingStore, + NULL, qcow2GetFeatures }, [VIR_STORAGE_FILE_QED] = { /* https://wiki.qemu.org/Features/QED */ 0, "QED", LV_LITTLE_ENDIAN, -2, 0, {0}, - QED_HDR_IMAGE_SIZE, 8, 1, NULL, NULL, qedGetBackingStore, NULL + QED_HDR_IMAGE_SIZE, 8, 1, NULL, NULL, qedGetBackingStore, NULL, NULL }, [VIR_STORAGE_FILE_VMDK] = { 0, "KDMV", LV_LITTLE_ENDIAN, 4, 4, {1, 2, 3}, - 4+4+4, 8, 512, NULL, NULL, vmdk4GetBackingStore, NULL + 4+4+4, 8, 512, NULL, NULL, vmdk4GetBackingStore, NULL, NULL }, }; G_STATIC_ASSERT(G_N_ELEMENTS(fileTypeInfo) == VIR_STORAGE_FILE_LAST); -- 2.43.5
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/storage_file/storage_file_probe.c | 53 +++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index 4792b9fdff..21430d18aa 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -106,6 +106,7 @@ qcow2GetClusterSize(const char *buf, size_t buf_size); static int qcowXGetBackingStore(char **, int *, const char *, size_t); +static int qcowXGetDataFile(char **, char *, int, size_t); static int qcow2GetFeatures(virBitmap **features, int format, char *buf, ssize_t len); static int vmdk4GetBackingStore(char **, int *, @@ -127,6 +128,7 @@ qedGetBackingStore(char **, int *, const char *, size_t); #define QCOW2_HDR_EXTENSION_END 0 #define QCOW2_HDR_EXTENSION_BACKING_FORMAT 0xE2792ACA +#define QCOW2_HDR_EXTENSION_DATA_FILE_NAME 0x44415441 #define QCOW2v3_HDR_FEATURES_INCOMPATIBLE (QCOW2_HDR_TOTAL_SIZE) #define QCOW2v3_HDR_FEATURES_COMPATIBLE (QCOW2v3_HDR_FEATURES_INCOMPATIBLE+8) @@ -314,7 +316,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { qcow2EncryptionInfo, qcow2GetClusterSize, qcowXGetBackingStore, - NULL, + qcowXGetDataFile, qcow2GetFeatures }, [VIR_STORAGE_FILE_QED] = { @@ -361,7 +363,7 @@ enum qcow2IncompatibleFeature { static const virStorageFileFeature qcow2IncompatibleFeatureArray[] = { VIR_STORAGE_FILE_FEATURE_LAST, /* QCOW2_INCOMPATIBLE_FEATURE_DIRTY */ VIR_STORAGE_FILE_FEATURE_LAST, /* QCOW2_INCOMPATIBLE_FEATURE_CORRUPT */ - VIR_STORAGE_FILE_FEATURE_LAST, /* QCOW2_INCOMPATIBLE_FEATURE_DATA_FILE */ + VIR_STORAGE_FILE_FEATURE_DATA_FILE, /* QCOW2_INCOMPATIBLE_FEATURE_DATA_FILE */ VIR_STORAGE_FILE_FEATURE_LAST, /* QCOW2_INCOMPATIBLE_FEATURE_COMPRESSION */ VIR_STORAGE_FILE_FEATURE_EXTENDED_L2, /* QCOW2_INCOMPATIBLE_FEATURE_EXTL2 */ }; @@ -393,7 +395,8 @@ cowGetBackingStore(char **res, static int qcow2GetExtensions(const char *buf, size_t buf_size, - int *backingFormat) + int *backingFormat, + char **dataFilePath) { size_t offset; size_t extension_start; @@ -488,6 +491,17 @@ qcow2GetExtensions(const char *buf, break; } + case QCOW2_HDR_EXTENSION_DATA_FILE_NAME: { + g_autofree char *tmp = NULL; + if (!dataFilePath) + break; + + *(char **)dataFilePath = g_new0(char, len + 1); + memcpy(*(char **)dataFilePath, buf + offset, len); + (*((char **)dataFilePath))[len] = '\0'; + break; + } + case QCOW2_HDR_EXTENSION_END: return 0; } @@ -554,9 +568,34 @@ qcowXGetBackingStore(char **res, memcpy(*res, buf + offset, size); (*res)[size] = '\0'; - if (qcow2GetExtensions(buf, buf_size, format) < 0) + if (qcow2GetExtensions(buf, buf_size, format, NULL) < 0) + return 0; + + return 0; +} + + +static int +qcowXGetDataFile(char **res, + char *buf, + int format, + size_t buf_size) +{ + virBitmap *features = NULL; + char *filename = NULL; + *res = NULL; + + if (buf_size < QCOW2v3_HDR_FEATURES_INCOMPATIBLE + 8) return 0; + ignore_value(qcow2GetFeatures(&features, format, buf, buf_size)); + if (features && virBitmapIsBitSet(features, VIR_STORAGE_FILE_FEATURE_DATA_FILE)) { + if (qcow2GetExtensions(buf, buf_size, NULL, &filename) < 0) + return 0; + + *res = g_strdup(filename); + } + return 0; } @@ -963,6 +1002,12 @@ virStorageFileProbeGetMetadata(virStorageSource *meta, meta->backingStoreRawFormat = format; } + VIR_FREE(meta->dataFileRaw); + if (fileTypeInfo[meta->format].getDataFile != NULL) { + fileTypeInfo[meta->format].getDataFile(&meta->dataFileRaw, + buf, meta->format, len); + } + g_clear_pointer(&meta->features, virBitmapFree); if (fileTypeInfo[meta->format].getFeatures != NULL && fileTypeInfo[meta->format].getFeatures(&meta->features, meta->format, buf, len) < 0) -- 2.43.5
On Sat, Sep 07, 2024 at 17:15:21 +0300, Nikolai Barybin via Devel wrote:
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/storage_file/storage_file_probe.c | 53 +++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 4 deletions(-)
diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index 4792b9fdff..21430d18aa 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -106,6 +106,7 @@ qcow2GetClusterSize(const char *buf, size_t buf_size); static int qcowXGetBackingStore(char **, int *, const char *, size_t); +static int qcowXGetDataFile(char **, char *, int, size_t);
data file is a qcow2 only feature so calling this 'qcowX' makes no sense as it won't be reused.
static int qcow2GetFeatures(virBitmap **features, int format, char *buf, ssize_t len); static int vmdk4GetBackingStore(char **, int *, @@ -393,7 +395,8 @@ cowGetBackingStore(char **res, static int qcow2GetExtensions(const char *buf, size_t buf_size, - int *backingFormat) + int *backingFormat, + char **dataFilePath) { size_t offset; size_t extension_start; @@ -488,6 +491,17 @@ qcow2GetExtensions(const char *buf, break; }
+ case QCOW2_HDR_EXTENSION_DATA_FILE_NAME: { + g_autofree char *tmp = NULL;
'tmp' is unused.
+ if (!dataFilePath) + break; + + *(char **)dataFilePath = g_new0(char, len + 1);
No need for that amount of typecasts since 'dataFilePath' is aready 'char **'
+ memcpy(*(char **)dataFilePath, buf + offset, len);
same here.
+ (*((char **)dataFilePath))[len] = '\0';
This is not needed as g_new0 allocates a zeroed-out buffer.
+ break; + } + case QCOW2_HDR_EXTENSION_END: return 0; } @@ -554,9 +568,34 @@ qcowXGetBackingStore(char **res, memcpy(*res, buf + offset, size); (*res)[size] = '\0';
- if (qcow2GetExtensions(buf, buf_size, format) < 0) + if (qcow2GetExtensions(buf, buf_size, format, NULL) < 0) + return 0; + + return 0; +} + + +static int +qcowXGetDataFile(char **res, + char *buf, + int format, + size_t buf_size) +{ + virBitmap *features = NULL; + char *filename = NULL; + *res = NULL; + + if (buf_size < QCOW2v3_HDR_FEATURES_INCOMPATIBLE + 8) return 0;
+ ignore_value(qcow2GetFeatures(&features, format, buf, buf_size));
'qcowXGetDataFile' gets called one block before 'qcow2GetFeatures' would be called via 'fileTypeInfo[meta->format].getFeatures(&meta->features,' but here you do it again. Restrucutre the code so that iut doesn't need to do this twice.
+ if (features && virBitmapIsBitSet(features, VIR_STORAGE_FILE_FEATURE_DATA_FILE)) { + if (qcow2GetExtensions(buf, buf_size, NULL, &filename) < 0)
So 'filename' will be filled with an allocated buffer ...
+ return 0; + + *res = g_strdup(filename);
... you'll dup it (which is wasteful) and then leak the original.
+ } + return 0; }
@@ -963,6 +1002,12 @@ virStorageFileProbeGetMetadata(virStorageSource *meta, meta->backingStoreRawFormat = format; }
+ VIR_FREE(meta->dataFileRaw); + if (fileTypeInfo[meta->format].getDataFile != NULL) { + fileTypeInfo[meta->format].getDataFile(&meta->dataFileRaw, + buf, meta->format, len); + } + g_clear_pointer(&meta->features, virBitmapFree); if (fileTypeInfo[meta->format].getFeatures != NULL && fileTypeInfo[meta->format].getFeatures(&meta->features, meta->format, buf, len) < 0) -- 2.43.5
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/storage_file/storage_source.c | 28 ++++++++++++++++++++++++++++ src/storage_file/storage_source.h | 3 +++ 2 files changed, 31 insertions(+) diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c index 2cfe3bc325..20d5b30c6c 100644 --- a/src/storage_file/storage_source.c +++ b/src/storage_file/storage_source.c @@ -543,6 +543,22 @@ virStorageSourceNewFromBacking(virStorageSource *parent, } +int virStorageSourceNewFromDataFile(virStorageSource *parent, + virStorageSource **dataFileSrc) +{ + int rc; + + if ((rc = virStorageSourceNewFromChild(parent, + parent->dataFileRaw, + dataFileSrc)) < 0) + return rc; + + (*dataFileSrc)->format = VIR_STORAGE_FILE_RAW; + (*dataFileSrc)->readonly = parent->readonly; + return rc; +} + + /** * @src: disk source definition structure * @fd: file descriptor @@ -1391,6 +1407,18 @@ virStorageSourceGetMetadataRecurse(virStorageSource *src, } } + if (src->dataFileRaw) { + g_autoptr(virStorageSource) dataFileStore = NULL; + if ((rv = virStorageSourceNewFromDataFile(src, &dataFileStore)) < 0) + return -1; + + /* the data file would not be usable for VM usage */ + if (rv == 1) + return 0; + + src->dataFileStore = g_steal_pointer(&dataFileStore); + } + if (src->backingStoreRaw) { if ((rv = virStorageSourceNewFromBacking(src, &backingStore)) < 0) return -1; diff --git a/src/storage_file/storage_source.h b/src/storage_file/storage_source.h index 63fefb6919..0514ff2364 100644 --- a/src/storage_file/storage_source.h +++ b/src/storage_file/storage_source.h @@ -72,6 +72,9 @@ int virStorageSourceNewFromBacking(virStorageSource *parent, virStorageSource **backing); +int virStorageSourceNewFromDataFile(virStorageSource *parent, + virStorageSource **dataFileSrc); + int virStorageSourceGetRelativeBackingPath(virStorageSource *top, virStorageSource *base, -- 2.43.5
On Sat, Sep 07, 2024 at 17:15:23 +0300, Nikolai Barybin via Devel wrote:
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/storage_file/storage_source.c | 28 ++++++++++++++++++++++++++++ src/storage_file/storage_source.h | 3 +++ 2 files changed, 31 insertions(+)
diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c index 2cfe3bc325..20d5b30c6c 100644 --- a/src/storage_file/storage_source.c +++ b/src/storage_file/storage_source.c @@ -543,6 +543,22 @@ virStorageSourceNewFromBacking(virStorageSource *parent, }
+int virStorageSourceNewFromDataFile(virStorageSource *parent, + virStorageSource **dataFileSrc)
This file uses the new coding style for headers where type is on a separate line. Please don't combine styles in this file. This is also missing a comment describing what the function does as with other functions in this file.
+{ + int rc; + + if ((rc = virStorageSourceNewFromChild(parent, + parent->dataFileRaw, + dataFileSrc)) < 0) + return rc; + + (*dataFileSrc)->format = VIR_STORAGE_FILE_RAW; + (*dataFileSrc)->readonly = parent->readonly;
You can do these at the beginning and then return the value from 'virStorageSourceNewFromChild' directly without need for 'rc'
+ return rc; +} + + /** * @src: disk source definition structure * @fd: file descriptor @@ -1391,6 +1407,18 @@ virStorageSourceGetMetadataRecurse(virStorageSource *src, } }
+ if (src->dataFileRaw) { + g_autoptr(virStorageSource) dataFileStore = NULL; + if ((rv = virStorageSourceNewFromDataFile(src, &dataFileStore)) < 0) + return -1; + + /* the data file would not be usable for VM usage */ + if (rv == 1) + return 0; + + src->dataFileStore = g_steal_pointer(&dataFileStore); + } + if (src->backingStoreRaw) { if ((rv = virStorageSourceNewFromBacking(src, &backingStore)) < 0) return -1; diff --git a/src/storage_file/storage_source.h b/src/storage_file/storage_source.h index 63fefb6919..0514ff2364 100644 --- a/src/storage_file/storage_source.h +++ b/src/storage_file/storage_source.h @@ -72,6 +72,9 @@ int virStorageSourceNewFromBacking(virStorageSource *parent, virStorageSource **backing);
+int virStorageSourceNewFromDataFile(virStorageSource *parent, + virStorageSource **dataFileSrc); +
same here, please use the existing coding style
int virStorageSourceGetRelativeBackingPath(virStorageSource *top, virStorageSource *base, -- 2.43.5
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/security/security_dac.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 59fc5b840f..f0dde46e25 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -951,6 +951,10 @@ virSecurityDACSetImageLabel(virSecurityManager *mgr, if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent, isChainTop) < 0) return -1; + if (n->dataFileStore && + virSecurityDACSetImageLabelInternal(mgr, def, n->dataFileStore, n, true) < 0) + return -1; + if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN)) break; @@ -1042,7 +1046,12 @@ virSecurityDACRestoreImageLabel(virSecurityManager *mgr, virStorageSource *src, virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED) { - return virSecurityDACRestoreImageLabelInt(mgr, def, src, false); + int rc = virSecurityDACRestoreImageLabelInt(mgr, def, src, false); + + if (rc == 0 && src->dataFileStore) + rc = virSecurityDACRestoreImageLabelInt(mgr, def, src->dataFileStore, false); + + return rc; } @@ -1906,10 +1915,17 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, def->name, migrated); for (i = 0; i < def->ndisks; i++) { - if (virSecurityDACRestoreImageLabelInt(mgr, - def, - def->disks[i]->src, - migrated) < 0) + int ret = virSecurityDACRestoreImageLabelInt(mgr, + def, + def->disks[i]->src, + migrated); + + if (ret == 0 && def->disks[i]->src->dataFileStore) + ret = virSecurityDACRestoreImageLabelInt(mgr, + def, + def->disks[i]->src->dataFileStore, + migrated); + if (ret < 0) rc = -1; } -- 2.43.5
On Sat, Sep 07, 2024 at 17:15:25 +0300, Nikolai Barybin via Devel wrote:
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/security/security_dac.c | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 59fc5b840f..f0dde46e25 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -951,6 +951,10 @@ virSecurityDACSetImageLabel(virSecurityManager *mgr, if (virSecurityDACSetImageLabelInternal(mgr, def, n, parent, isChainTop) < 0) return -1;
+ if (n->dataFileStore &&
You "arbitrarily" chose to set 'isChainTop' as true instead of passing the real value. While I do understand why (the data file can't be shared anyways so it can't be used by anything else, thus we need to consider it with teh same permissions as the top image), it's not clear for random readers why. Add a comment and explain it.
+ virSecurityDACSetImageLabelInternal(mgr, def, n->dataFileStore, n, true) < 0) + return -1; + if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN)) break;
@@ -1042,7 +1046,12 @@ virSecurityDACRestoreImageLabel(virSecurityManager *mgr, virStorageSource *src, virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED) { - return virSecurityDACRestoreImageLabelInt(mgr, def, src, false); + int rc = virSecurityDACRestoreImageLabelInt(mgr, def, src, false); + + if (rc == 0 && src->dataFileStore) + rc = virSecurityDACRestoreImageLabelInt(mgr, def, src->dataFileStore, false); + + return rc;
Please use the more common way to do this: if (virSecurityDACRestoreImageLabelInt(mgr, def, src, false) < 0) return -1; if (src->dataFileStore && virSecurityDACRestoreImageLabelInt(mgr, def, src->dataFileStore, false) < 0) return -1; return 0; I've also considered whether the data file seclabel shouldn't be restored even if the restoration of the normal part fails. Given that I don't think this will be used much I guess we should be okay even like this.
@@ -1906,10 +1915,17 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, def->name, migrated);
for (i = 0; i < def->ndisks; i++) { - if (virSecurityDACRestoreImageLabelInt(mgr, - def, - def->disks[i]->src, - migrated) < 0) + int ret = virSecurityDACRestoreImageLabelInt(mgr, + def, + def->disks[i]->src, + migrated); + + if (ret == 0 && def->disks[i]->src->dataFileStore) + ret = virSecurityDACRestoreImageLabelInt(mgr, + def, + def->disks[i]->src->dataFileStore, + migrated); + if (ret < 0) rc = -1;
Use logic as above ... e.g. don't use 'ret'
}
-- 2.43.5
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/security/security_selinux.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 31df4d22db..6d0611fe50 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1871,7 +1871,12 @@ virSecuritySELinuxRestoreImageLabel(virSecurityManager *mgr, virStorageSource *src, virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED) { - return virSecuritySELinuxRestoreImageLabelInt(mgr, def, src, false); + int rc = virSecuritySELinuxRestoreImageLabelInt(mgr, def, src, false); + + if (rc == 0 && src->dataFileStore) + rc = virSecuritySELinuxRestoreImageLabelInt(mgr, def, src->dataFileStore, false); + + return rc; } @@ -1996,6 +2001,10 @@ virSecuritySELinuxSetImageLabel(virSecurityManager *mgr, if (virSecuritySELinuxSetImageLabelInternal(mgr, def, n, parent, isChainTop) < 0) return -1; + if (n->dataFileStore && + virSecuritySELinuxSetImageLabelInternal(mgr, def, n->dataFileStore, parent, isChainTop) < 0) + return -1; + if (!(flags & VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN)) break; @@ -2843,9 +2852,12 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, for (i = 0; i < def->ndisks; i++) { virDomainDiskDef *disk = def->disks[i]; - - if (virSecuritySELinuxRestoreImageLabelInt(mgr, def, disk->src, - migrated) < 0) + int ret = virSecuritySELinuxRestoreImageLabelInt(mgr, def, disk->src, + migrated); + if (ret == 0 && disk->src->dataFileStore) + ret = virSecuritySELinuxRestoreImageLabelInt(mgr, def, disk->src->dataFileStore, + migrated); + if (ret < 0) rc = -1; } -- 2.43.5
On Sat, Sep 07, 2024 at 17:15:27 +0300, Nikolai Barybin via Devel wrote:
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/security/security_selinux.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
Same comments as in: [PATCH v2 05/13] security: DAC: handle qcow2 data-file on image label set/restore
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/security/virt-aa-helper.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 067a17f331..4e8334eb3e 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -877,6 +877,10 @@ storage_source_add_files(virStorageSource *src, if (add_file_path(tmp, depth, buf) < 0) return -1; + if (src->dataFileStore && + storage_source_add_files(src->dataFileStore, buf, 0) < 0) + return -1; + depth++; } -- 2.43.5
On Sat, Sep 07, 2024 at 17:15:29 +0300, Nikolai Barybin via Devel wrote:
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/security/virt-aa-helper.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 067a17f331..4e8334eb3e 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -877,6 +877,10 @@ storage_source_add_files(virStorageSource *src, if (add_file_path(tmp, depth, buf) < 0) return -1;
+ if (src->dataFileStore && + storage_source_add_files(src->dataFileStore, buf, 0) < 0) + return -1;
As this won't ever have backing files and for symetry with the other security drivers you should use add_file_path here instead as that doesn't try to iterate the backing chain.
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/qemu/qemu_cgroup.c | 4 ++++ src/qemu/qemu_namespace.c | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 23b7e6b4e8..a6d3c16e50 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -223,6 +223,10 @@ qemuSetupImageCgroupInternal(virDomainObj *vm, qemuSetupImagePathCgroup(vm, QEMU_DEVICE_MAPPER_CONTROL_PATH, false) < 0) return -1; + if (src->dataFileStore && + qemuSetupImagePathCgroup(vm, src->dataFileStore->path, readonly) < 0) + return -1; + return qemuSetupImagePathCgroup(vm, path, readonly); } diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index bbe3d5a1f7..afeec63a4f 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -290,6 +290,11 @@ qemuDomainSetupDisk(virStorageSource *src, if (targetPaths) *paths = g_slist_concat(g_slist_reverse(targetPaths), *paths); + + if (next->dataFileStore) { + g_autofree char *dataFilePath = g_strdup(next->dataFileStore->path); + *paths = g_slist_prepend(*paths, g_steal_pointer(&dataFilePath)); + } } *paths = g_slist_prepend(*paths, g_steal_pointer(&tmpPath)); -- 2.43.5
On Sat, Sep 07, 2024 at 17:15:31 +0300, Nikolai Barybin via Devel wrote:
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/qemu/qemu_cgroup.c | 4 ++++ src/qemu/qemu_namespace.c | 5 +++++ 2 files changed, 9 insertions(+)
Firstly, that applies everywhere, you must not use the 'path' member of virStorageSource as a local file path unless you check that it's a local source by calling virStorageSourceIsLocalStorage. Network storage backends (e.g. NBD) use that field for the relative path/export name so it's not a local path then.
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 23b7e6b4e8..a6d3c16e50 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -223,6 +223,10 @@ qemuSetupImageCgroupInternal(virDomainObj *vm, qemuSetupImagePathCgroup(vm, QEMU_DEVICE_MAPPER_CONTROL_PATH, false) < 0) return -1;
+ if (src->dataFileStore && + qemuSetupImagePathCgroup(vm, src->dataFileStore->path, readonly) < 0) + return -1; +
I don't think this is correct. You should call 'qemuSetupImageCgroupInternal' on the data file virStorageSource struct instead, rather than setup it partially inside.
return qemuSetupImagePathCgroup(vm, path, readonly); }
diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index bbe3d5a1f7..afeec63a4f 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -290,6 +290,11 @@ qemuDomainSetupDisk(virStorageSource *src,
if (targetPaths) *paths = g_slist_concat(g_slist_reverse(targetPaths), *paths); + + if (next->dataFileStore) { + g_autofree char *dataFilePath = g_strdup(next->dataFileStore->path); + *paths = g_slist_prepend(*paths, g_steal_pointer(&dataFilePath)); + }
And this isn't correct either because this code will be skipped if the QCOW2 bit (the metadata file) is not stored locally.
}
*paths = g_slist_prepend(*paths, g_steal_pointer(&tmpPath)); -- 2.43.5
This refactoring will simplify next changes. Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/qemu/qemu_domain.c | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ed305d9427..86362393e2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7974,6 +7974,32 @@ qemuDomainPrepareStorageSourceConfig(virStorageSource *src, } +static int +qemuDomainPrepareStorageSource(virStorageSource *src, + virDomainObj *vm, + virDomainDiskDef *disk, + virQEMUDriverConfig *cfg) +{ + qemuDomainObjPrivate *priv = vm->privateData; + + /* convert detected ISO format to 'raw' as qemu would not understand it */ + if (src->format == VIR_STORAGE_FILE_ISO) + src->format = VIR_STORAGE_FILE_RAW; + + if (qemuDomainValidateStorageSource(src, priv->qemuCaps) < 0) + return -1; + + qemuDomainPrepareStorageSourceConfig(src, cfg); + qemuDomainPrepareDiskSourceData(disk, src); + + if (!qemuDiskBusIsSD(disk->bus) && + qemuDomainPrepareStorageSourceBlockdev(disk, src, priv, cfg) < 0) + return -1; + + return 0; +} + + /** * qemuDomainDetermineDiskChain: * @driver: qemu driver object @@ -7994,7 +8020,6 @@ qemuDomainDetermineDiskChain(virQEMUDriver *driver, g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virStorageSource *src; /* iterator for the backing chain declared in XML */ virStorageSource *n; /* iterator for the backing chain detected from disk */ - qemuDomainObjPrivate *priv = vm->privateData; uid_t uid; gid_t gid; @@ -8078,18 +8103,7 @@ qemuDomainDetermineDiskChain(virQEMUDriver *driver, return -1; for (n = src->backingStore; virStorageSourceIsBacking(n); n = n->backingStore) { - /* convert detected ISO format to 'raw' as qemu would not understand it */ - if (n->format == VIR_STORAGE_FILE_ISO) - n->format = VIR_STORAGE_FILE_RAW; - - if (qemuDomainValidateStorageSource(n, priv->qemuCaps) < 0) - return -1; - - qemuDomainPrepareStorageSourceConfig(n, cfg); - qemuDomainPrepareDiskSourceData(disk, n); - - if (!qemuDiskBusIsSD(disk->bus) && - qemuDomainPrepareStorageSourceBlockdev(disk, n, priv, cfg) < 0) + if (qemuDomainPrepareStorageSource(n, vm, disk, cfg) < 0) return -1; } -- 2.43.5
On Sat, Sep 07, 2024 at 17:15:33 +0300, Nikolai Barybin via Devel wrote:
This refactoring will simplify next changes.
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/qemu/qemu_domain.c | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
- propogate data-file to cmdline - determine data-file within disk chain - enable live disk insertion Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/qemu/qemu_block.c | 7 +++++++ src/qemu/qemu_command.c | 5 +++++ src/qemu/qemu_domain.c | 14 +++++++++++++- 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 6e90bae9f2..5d2a638a56 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1301,6 +1301,13 @@ qemuBlockStorageSourceGetFormatQcow2Props(virStorageSource *src, NULL) < 0) return -1; + if (src->dataFileStore) { + if (virJSONValueObjectAdd(&props, + "s:data-file", src->dataFileStore->nodenameformat, + NULL) < 0) + return -1; + } + return 0; } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1b992d8eed..503374f470 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -11178,6 +11178,11 @@ qemuBuildStorageSourceChainAttachPrepareBlockdev(virStorageSource *top) if (qemuBuildStorageSourceChainAttachPrepareBlockdevOne(data, n, n->backingStore) < 0) return NULL; + + if (n->dataFileStore && + qemuBuildStorageSourceChainAttachPrepareBlockdevOne(data, n->dataFileStore, + n->dataFileStore->backingStore) < 0) + return NULL; } return g_steal_pointer(&data); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 86362393e2..0555bc8cfd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8102,9 +8102,17 @@ qemuDomainDetermineDiskChain(virQEMUDriver *driver, true) < 0) return -1; + if (src->dataFileStore && + qemuDomainPrepareStorageSource(src->dataFileStore, vm, disk, cfg) < 0) + return -1; + for (n = src->backingStore; virStorageSourceIsBacking(n); n = n->backingStore) { if (qemuDomainPrepareStorageSource(n, vm, disk, cfg) < 0) return -1; + + if (n->dataFileStore && + qemuDomainPrepareStorageSource(n->dataFileStore, vm, disk, cfg) < 0) + return -1; } if (qemuDomainStorageSourceValidateDepth(disksrc, 0, disk->dst) < 0) @@ -11298,7 +11306,7 @@ qemuDomainPrepareDiskSourceData(virDomainDiskDef *disk, return; /* transfer properties valid only for the top level image */ - if (src == disk->src) + if (src == disk->src || src == disk->src->dataFileStore) src->detect_zeroes = disk->detect_zeroes; /* transfer properties valid for the full chain */ @@ -11527,6 +11535,10 @@ qemuDomainPrepareDiskSourceBlockdev(virDomainDiskDef *disk, for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { if (qemuDomainPrepareStorageSourceBlockdev(disk, n, priv, cfg) < 0) return -1; + + if (n->dataFileStore && + qemuDomainPrepareStorageSourceBlockdev(disk, n->dataFileStore, priv, cfg) < 0) + return -1; } return 0; -- 2.43.5
On Sat, Sep 07, 2024 at 17:15:35 +0300, Nikolai Barybin via Devel wrote:
- propogate data-file to cmdline - determine data-file within disk chain - enable live disk insertion
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/qemu/qemu_block.c | 7 +++++++ src/qemu/qemu_command.c | 5 +++++ src/qemu/qemu_domain.c | 14 +++++++++++++- 3 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 6e90bae9f2..5d2a638a56 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1301,6 +1301,13 @@ qemuBlockStorageSourceGetFormatQcow2Props(virStorageSource *src, NULL) < 0) return -1;
+ if (src->dataFileStore) { + if (virJSONValueObjectAdd(&props, + "s:data-file", src->dataFileStore->nodenameformat,
You must not use the nodename* fields directly. In fact this doesn't even work as raw files don't even have a 'format' node name any more. This needs to be qemuBlockStorageSourceGetEffectiveNodename(src->dataFileStore)
+ NULL) < 0) + return -1; + } + return 0; }
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1b992d8eed..503374f470 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -11178,6 +11178,11 @@ qemuBuildStorageSourceChainAttachPrepareBlockdev(virStorageSource *top) if (qemuBuildStorageSourceChainAttachPrepareBlockdevOne(data, n, n->backingStore) < 0) return NULL; + + if (n->dataFileStore && + qemuBuildStorageSourceChainAttachPrepareBlockdevOne(data, n->dataFileStore, + n->dataFileStore->backingStore) < 0) + return NULL; }
Missing corresponding changes to qemuBlockStorageSourceChainDetachPrepareBlockdev(), thus when hot-unplugging a disk the data file will not be unplugged.
return g_steal_pointer(&data);
Possibly as a separata patch you'll also need to forbid image creation with data file, e.g. when creating snapshots. 'qemuBlockStorageSourceCreate' is the function to consider.
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/conf/schemas/domaincommon.rng | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index efb5f00d77..b2fe20cdd4 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -1778,6 +1778,9 @@ <ref name="diskBackingChain"/> <ref name="privateDataDeviceDisk"/> </interleave> + <optional> + <ref name="diskDataFile"/> + </optional> </define> <define name="diskBackingChain"> @@ -1798,6 +1801,18 @@ <ref name="diskSource"/> <ref name="diskBackingChain"/> <ref name="diskFormat"/> + <optional> + <ref name="diskDataFile"/> + </optional> + </interleave> + </element> + </define> + + <define name="diskDataFile"> + <element name="dataFileStore"> + <interleave> + <ref name="diskFormat"/> + <ref name="diskSource"/> </interleave> </element> </define> -- 2.43.5
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/conf/domain_conf.c | 98 ++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 13 ++++++ 2 files changed, 111 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a263612ef7..cfd25032b3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7654,6 +7654,52 @@ virDomainStorageSourceParse(xmlNodePtr node, } +int +virDomainDiskDataFileStoreParse(xmlXPathContextPtr ctxt, + virStorageSource *src, + unsigned int flags, + virDomainXMLOption *xmlopt) +{ + VIR_XPATH_NODE_AUTORESTORE(ctxt) + xmlNodePtr source; + g_autofree char *type = NULL; + g_autofree char *format = NULL; + g_autoptr(virStorageSource) dataFileStore = NULL; + + if (!(ctxt->node = virXPathNode("./dataFileStore", ctxt))) + return 0; + + if (!(type = virXMLPropString(ctxt->node, "type"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing data file store type")); + return -1; + } + + if (!(format = virXPathString("string(./format/@type)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing data file store format")); + return -1; + } + + if (!(source = virXPathNode("./source", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing disk data file store source")); + return -1; + } + + if (!(dataFileStore = virDomainStorageSourceParseBase(type, format, NULL))) + return -1; + + if (virDomainStorageSourceParse(source, ctxt, dataFileStore, flags, xmlopt) < 0) + return -1; + + dataFileStore->readonly = src->readonly; + src->dataFileStore = g_steal_pointer(&dataFileStore); + + return 0; +} + + int virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, virStorageSource *src, @@ -7704,6 +7750,7 @@ virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, backingStore->readonly = true; if (virDomainStorageSourceParse(source, ctxt, backingStore, flags, xmlopt) < 0 || + virDomainDiskDataFileStoreParse(ctxt, backingStore, flags, xmlopt) < 0 || virDomainDiskBackingStoreParse(ctxt, backingStore, flags, xmlopt) < 0) return -1; @@ -8096,6 +8143,9 @@ virDomainDiskDefParseSourceXML(virDomainXMLOption *xmlopt, return NULL; } + if (virDomainDiskDataFileStoreParse(ctxt, src, flags, xmlopt) < 0) + return NULL; + if (virDomainDiskBackingStoreParse(ctxt, src, flags, xmlopt) < 0) return NULL; @@ -22819,6 +22869,46 @@ virDomainDiskSourceFormat(virBuffer *buf, return 0; } +int +virDomainDiskDataFileStoreFormat(virBuffer *buf, + virStorageSource *src, + virDomainXMLOption *xmlopt, + unsigned int flags) +{ + g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) formatAttrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); + virStorageSource *dataFileStore = src->dataFileStore; + bool inactive = flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE; + + if (!dataFileStore) + return 0; + + /* don't write detected data file member to inactive xml */ + if (inactive && dataFileStore->detected) + return 0; + + if (dataFileStore->format != VIR_STORAGE_FILE_RAW) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected disk data file format '%1$d', only raw format is supported"), + dataFileStore->format); + return -1; + } + + virBufferAsprintf(&attrBuf, " type='%s'", + virStorageTypeToString(dataFileStore->type)); + + virBufferAsprintf(&formatAttrBuf, " type='%s'", + virStorageFileFormatTypeToString(dataFileStore->format)); + virXMLFormatElement(&childBuf, "format", &formatAttrBuf, NULL); + + if (virDomainDiskSourceFormat(&childBuf, dataFileStore, "source", 0, false, + flags, false, false, xmlopt) < 0) + return -1; + + virXMLFormatElement(buf, "dataFileStore", &attrBuf, &childBuf); + return 0; +} int virDomainDiskBackingStoreFormat(virBuffer *buf, @@ -22877,6 +22967,10 @@ virDomainDiskBackingStoreFormat(virBuffer *buf, flags, false, false, xmlopt) < 0) return -1; + if (backingStore->dataFileStore && + virDomainDiskDataFileStoreFormat(&childBuf, backingStore, xmlopt, flags) < 0) + return -1; + if (virDomainDiskBackingStoreFormat(&childBuf, backingStore, xmlopt, flags) < 0) return -1; @@ -23195,6 +23289,10 @@ virDomainDiskDefFormat(virBuffer *buf, if (virDomainDiskBackingStoreFormat(&childBuf, def->src, xmlopt, flags) < 0) return -1; + if (def->src->dataFileStore && + virDomainDiskDataFileStoreFormat(&childBuf, def->src, xmlopt, flags) < 0) + return -1; + virBufferEscapeString(&childBuf, "<backenddomain name='%s'/>\n", def->domain_name); virDomainDiskGeometryDefFormat(&childBuf, def); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 659299bdd1..993c49a731 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3884,6 +3884,12 @@ int virDomainDiskSourceFormat(virBuffer *buf, bool skipEnc, virDomainXMLOption *xmlopt); +int +virDomainDiskDataFileStoreFormat(virBuffer *buf, + virStorageSource *src, + virDomainXMLOption *xmlopt, + unsigned int flags); + int virDomainDiskBackingStoreFormat(virBuffer *buf, virStorageSource *src, @@ -4399,6 +4405,13 @@ int virDomainStorageSourceParse(xmlNodePtr node, virDomainXMLOption *xmlopt) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +int +virDomainDiskDataFileStoreParse(xmlXPathContextPtr ctxt, + virStorageSource *src, + unsigned int flags, + virDomainXMLOption *xmlopt) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; + int virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, virStorageSource *src, -- 2.43.5
On Sat, Sep 07, 2024 at 17:15:40 +0300, Nikolai Barybin via Devel wrote: As noted separately, tests and documentation is missing.
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- src/conf/domain_conf.c | 98 ++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 13 ++++++ 2 files changed, 111 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a263612ef7..cfd25032b3 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7654,6 +7654,52 @@ virDomainStorageSourceParse(xmlNodePtr node, }
+int +virDomainDiskDataFileStoreParse(xmlXPathContextPtr ctxt, + virStorageSource *src, + unsigned int flags, + virDomainXMLOption *xmlopt) +{ + VIR_XPATH_NODE_AUTORESTORE(ctxt) + xmlNodePtr source; + g_autofree char *type = NULL; + g_autofree char *format = NULL; + g_autoptr(virStorageSource) dataFileStore = NULL; + + if (!(ctxt->node = virXPathNode("./dataFileStore", ctxt))) + return 0; + + if (!(type = virXMLPropString(ctxt->node, "type"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing data file store type")); + return -1; + }
You can use virXMLPropStringRequired() which also reports error.
+ + if (!(format = virXPathString("string(./format/@type)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing data file store format")); + return -1; + }
IMO we shouldn't even have a 'format' field here as this doesn't make sense to have with anything non-raw.
+ + if (!(source = virXPathNode("./source", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing disk data file store source")); + return -1; + } + + if (!(dataFileStore = virDomainStorageSourceParseBase(type, format, NULL))) + return -1; + + if (virDomainStorageSourceParse(source, ctxt, dataFileStore, flags, xmlopt) < 0) + return -1;
This parses too much, some of which we don't want to support with the data file, such as 'slice'. Make sure to forbid slices if they are parsed.
+ + dataFileStore->readonly = src->readonly; + src->dataFileStore = g_steal_pointer(&dataFileStore); + + return 0; +} + + int virDomainDiskBackingStoreParse(xmlXPathContextPtr ctxt, virStorageSource *src,
[...]
@@ -22819,6 +22869,46 @@ virDomainDiskSourceFormat(virBuffer *buf, return 0; }
+int +virDomainDiskDataFileStoreFormat(virBuffer *buf, + virStorageSource *src, + virDomainXMLOption *xmlopt, + unsigned int flags) +{ + g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) formatAttrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); + virStorageSource *dataFileStore = src->dataFileStore; + bool inactive = flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE; + + if (!dataFileStore) + return 0; + + /* don't write detected data file member to inactive xml */ + if (inactive && dataFileStore->detected) + return 0; + + if (dataFileStore->format != VIR_STORAGE_FILE_RAW) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected disk data file format '%1$d', only raw format is supported"), + dataFileStore->format);
This definitely doesn't belong to the formatter, but rather to the parser, where such a check is missing.
+ return -1; + } + + virBufferAsprintf(&attrBuf, " type='%s'", + virStorageTypeToString(dataFileStore->type)); + + virBufferAsprintf(&formatAttrBuf, " type='%s'", + virStorageFileFormatTypeToString(dataFileStore->format));
I wonder wheter we should even parse it if this only ever makes sense with raw images.
+ virXMLFormatElement(&childBuf, "format", &formatAttrBuf, NULL); + + if (virDomainDiskSourceFormat(&childBuf, dataFileStore, "source", 0, false, + flags, false, false, xmlopt) < 0) + return -1; + + virXMLFormatElement(buf, "dataFileStore", &attrBuf, &childBuf); + return 0; +}
int virDomainDiskBackingStoreFormat(virBuffer *buf,
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- 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 ++++++ 7 files changed, 219 insertions(+), 31 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 diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index c581bd1748..46c080fb3b 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -247,6 +247,48 @@ testQemuDiskXMLToJSONFakeSecrets(virStorageSource *src) static const char *testQemuDiskXMLToJSONPath = abs_srcdir "/qemublocktestdata/xml2json/"; +static int +testQemuDiskXMLToPropsCollectDataOne(virStorageSource *src, + virDomainDiskDef *disk, + struct testQemuDiskXMLToJSONData *data) +{ + g_autofree char *backingstore = NULL; + g_autoptr(virJSONValue) formatProps = NULL; + g_autoptr(virJSONValue) storageProps = NULL; + g_autoptr(virJSONValue) storageSrcOnlyProps = NULL; + + if (testQemuDiskXMLToJSONFakeSecrets(src) < 0) + return -1; + + if (qemuDomainValidateStorageSource(src, data->qemuCaps) < 0) + return -1; + + qemuDomainPrepareDiskSourceData(disk, src); + + if (!(formatProps = qemuBlockStorageSourceGetFormatProps(src, src->backingStore)) || + !(storageSrcOnlyProps = qemuBlockStorageSourceGetBackendProps(src, QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY)) || + !(storageProps = qemuBlockStorageSourceGetBackendProps(src, 0)) || + !(backingstore = qemuBlockGetBackingStoreString(src, true))) { + if (!data->fail) { + VIR_TEST_VERBOSE("failed to generate qemu blockdev props"); + return -1; + } + } else if (data->fail) { + VIR_TEST_VERBOSE("qemu blockdev props should have failed"); + return -1; + } + + VIR_REALLOC_N(data->images, data->nimages + 1); + + data->images[data->nimages].formatprops = g_steal_pointer(&formatProps); + data->images[data->nimages].storageprops = g_steal_pointer(&storageProps); + data->images[data->nimages].storagepropssrc = g_steal_pointer(&storageSrcOnlyProps); + data->images[data->nimages].backingstore = g_steal_pointer(&backingstore); + + data->nimages++; + return 0; +} + static int testQemuDiskXMLToProps(const void *opaque) { @@ -254,9 +296,6 @@ testQemuDiskXMLToProps(const void *opaque) g_autoptr(virDomainDef) vmdef = NULL; virDomainDiskDef *disk = NULL; virStorageSource *n; - g_autoptr(virJSONValue) formatProps = NULL; - g_autoptr(virJSONValue) storageProps = NULL; - g_autoptr(virJSONValue) storageSrcOnlyProps = NULL; g_autofree char *xmlpath = NULL; g_autofree char *xmlstr = NULL; @@ -284,37 +323,12 @@ testQemuDiskXMLToProps(const void *opaque) } for (n = disk->src; virStorageSourceIsBacking(n); n = n->backingStore) { - g_autofree char *backingstore = NULL; - - if (testQemuDiskXMLToJSONFakeSecrets(n) < 0) - return -1; - - if (qemuDomainValidateStorageSource(n, data->qemuCaps) < 0) + if (testQemuDiskXMLToPropsCollectDataOne(n, disk, data) < 0) return -1; - qemuDomainPrepareDiskSourceData(disk, n); - - if (!(formatProps = qemuBlockStorageSourceGetFormatProps(n, n->backingStore)) || - !(storageSrcOnlyProps = qemuBlockStorageSourceGetBackendProps(n, QEMU_BLOCK_STORAGE_SOURCE_BACKEND_PROPS_TARGET_ONLY)) || - !(storageProps = qemuBlockStorageSourceGetBackendProps(n, 0)) || - !(backingstore = qemuBlockGetBackingStoreString(n, true))) { - if (!data->fail) { - VIR_TEST_VERBOSE("failed to generate qemu blockdev props"); - return -1; - } - } else if (data->fail) { - VIR_TEST_VERBOSE("qemu blockdev props should have failed"); + if (n->dataFileStore && + testQemuDiskXMLToPropsCollectDataOne(n->dataFileStore, disk, data) < 0) return -1; - } - - VIR_REALLOC_N(data->images, data->nimages + 1); - - data->images[data->nimages].formatprops = g_steal_pointer(&formatProps); - data->images[data->nimages].storageprops = g_steal_pointer(&storageProps); - data->images[data->nimages].storagepropssrc = g_steal_pointer(&storageSrcOnlyProps); - data->images[data->nimages].backingstore = g_steal_pointer(&backingstore); - - data->nimages++; } return 0; @@ -1123,6 +1137,8 @@ mymain(void) TEST_DISK_TO_JSON("file-qcow2-backing-chain-unterminated"); TEST_DISK_TO_JSON("file-qcow2-backing-chain-encryption"); TEST_DISK_TO_JSON("network-qcow2-backing-chain-encryption_auth"); + TEST_DISK_TO_JSON("file-qcow2-data-file-noopts"); + TEST_DISK_TO_JSON("file-qcow2-backing-with-data-file-noopts"); TEST_DISK_TO_JSON("file-backing_basic-unmap"); TEST_DISK_TO_JSON("file-backing_basic-unmap-detect"); diff --git a/tests/qemublocktestdata/xml2json/file-qcow2-backing-with-data-file-noopts-srconly.json b/tests/qemublocktestdata/xml2json/file-qcow2-backing-with-data-file-noopts-srconly.json new file mode 100644 index 0000000000..1621b4b0fc --- /dev/null +++ b/tests/qemublocktestdata/xml2json/file-qcow2-backing-with-data-file-noopts-srconly.json @@ -0,0 +1,27 @@ +( + source only properties: + { + "driver": "file", + "filename": "/var/lib/libvirt/images/rhel7.3.1507297895" + } + backing store string: + /var/lib/libvirt/images/rhel7.3.1507297895 +) +( + source only properties: + { + "driver": "file", + "filename": "/var/lib/libvirt/images/rhel7.3.1484071872" + } + backing store string: + /var/lib/libvirt/images/rhel7.3.1484071872 +) +( + source only properties: + { + "driver": "file", + "filename": "/var/lib/libvirt/images/rhel7.3.1483615252" + } + backing store string: + /var/lib/libvirt/images/rhel7.3.1483615252 +) diff --git a/tests/qemublocktestdata/xml2json/file-qcow2-backing-with-data-file-noopts.json b/tests/qemublocktestdata/xml2json/file-qcow2-backing-with-data-file-noopts.json new file mode 100644 index 0000000000..b78848ddea --- /dev/null +++ b/tests/qemublocktestdata/xml2json/file-qcow2-backing-with-data-file-noopts.json @@ -0,0 +1,41 @@ +{ + "node-name": "#block126", + "read-only": false, + "driver": "qcow2", + "file": "#block004", + "backing": "#block313" +} +{ + "driver": "file", + "filename": "/var/lib/libvirt/images/rhel7.3.1507297895", + "node-name": "#block004", + "auto-read-only": true, + "discard": "unmap" +} +{ + "node-name": "#block313", + "read-only": true, + "driver": "qcow2", + "data-file": "#block556", + "file": "#block256" +} +{ + "driver": "file", + "filename": "/var/lib/libvirt/images/rhel7.3.1484071872", + "node-name": "#block256", + "auto-read-only": true, + "discard": "unmap" +} +{ + "node-name": "#block556", + "read-only": true, + "driver": "raw", + "file": "#block418" +} +{ + "driver": "file", + "filename": "/var/lib/libvirt/images/rhel7.3.1483615252", + "node-name": "#block418", + "auto-read-only": true, + "discard": "unmap" +} diff --git a/tests/qemublocktestdata/xml2json/file-qcow2-backing-with-data-file-noopts.xml b/tests/qemublocktestdata/xml2json/file-qcow2-backing-with-data-file-noopts.xml new file mode 100644 index 0000000000..4fd561b8bf --- /dev/null +++ b/tests/qemublocktestdata/xml2json/file-qcow2-backing-with-data-file-noopts.xml @@ -0,0 +1,35 @@ +<disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/var/lib/libvirt/images/rhel7.3.1507297895'> + <privateData> + <nodenames> + <nodename type='storage' name='#block004'/> + <nodename type='format' name='#block126'/> + </nodenames> + </privateData> + </source> + <backingStore type='file' index='1'> + <format type='qcow2'/> + <source file='/var/lib/libvirt/images/rhel7.3.1484071872'> + <privateData> + <nodenames> + <nodename type='storage' name='#block256'/> + <nodename type='format' name='#block313'/> + </nodenames> + </privateData> + </source> + <dataFileStore type='file'> + <format type='raw'/> + <source file='/var/lib/libvirt/images/rhel7.3.1483615252'> + <privateData> + <nodenames> + <nodename type='storage' name='#block418'/> + <nodename type='format' name='#block556'/> + </nodenames> + </privateData> + </source> + </dataFileStore> + </backingStore> + <target dev='vda' bus='virtio'/> + <alias name='virtio-disk0'/> +</disk> diff --git a/tests/qemublocktestdata/xml2json/file-qcow2-data-file-noopts-srconly.json b/tests/qemublocktestdata/xml2json/file-qcow2-data-file-noopts-srconly.json new file mode 100644 index 0000000000..f2fd81184b --- /dev/null +++ b/tests/qemublocktestdata/xml2json/file-qcow2-data-file-noopts-srconly.json @@ -0,0 +1,18 @@ +( + source only properties: + { + "driver": "file", + "filename": "/var/lib/libvirt/images/rhel7.3.1507297895" + } + backing store string: + /var/lib/libvirt/images/rhel7.3.1507297895 +) +( + source only properties: + { + "driver": "file", + "filename": "/var/lib/libvirt/images/rhel7.3.1484071872" + } + backing store string: + /var/lib/libvirt/images/rhel7.3.1484071872 +) diff --git a/tests/qemublocktestdata/xml2json/file-qcow2-data-file-noopts.json b/tests/qemublocktestdata/xml2json/file-qcow2-data-file-noopts.json new file mode 100644 index 0000000000..ed7782b4f6 --- /dev/null +++ b/tests/qemublocktestdata/xml2json/file-qcow2-data-file-noopts.json @@ -0,0 +1,27 @@ +{ + "node-name": "#block126", + "read-only": false, + "driver": "qcow2", + "data-file": "#block313", + "file": "#block004" +} +{ + "driver": "file", + "filename": "/var/lib/libvirt/images/rhel7.3.1507297895", + "node-name": "#block004", + "auto-read-only": true, + "discard": "unmap" +} +{ + "node-name": "#block313", + "read-only": false, + "driver": "raw", + "file": "#block256" +} +{ + "driver": "file", + "filename": "/var/lib/libvirt/images/rhel7.3.1484071872", + "node-name": "#block256", + "auto-read-only": true, + "discard": "unmap" +} diff --git a/tests/qemublocktestdata/xml2json/file-qcow2-data-file-noopts.xml b/tests/qemublocktestdata/xml2json/file-qcow2-data-file-noopts.xml new file mode 100644 index 0000000000..6d4c9fd174 --- /dev/null +++ b/tests/qemublocktestdata/xml2json/file-qcow2-data-file-noopts.xml @@ -0,0 +1,24 @@ +<disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <source file='/var/lib/libvirt/images/rhel7.3.1507297895'> + <privateData> + <nodenames> + <nodename type='storage' name='#block004'/> + <nodename type='format' name='#block126'/> + </nodenames> + </privateData> + </source> + <dataFileStore type='file'> + <format type='raw'/> + <source file='/var/lib/libvirt/images/rhel7.3.1484071872'> + <privateData> + <nodenames> + <nodename type='storage' name='#block256'/> + <nodename type='format' name='#block313'/> + </nodenames> + </privateData> + </source> + </dataFileStore> + <target dev='vda' bus='virtio'/> + <alias name='virtio-disk0'/> +</disk> \ No newline at end of file -- 2.43.5
On Sat, Sep 07, 2024 at 17:15:42 +0300, Nikolai Barybin via Devel wrote:
Signed-off-by: Nikolai Barybin <nikolai.barybin@virtuozzo.com> --- 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 ++++++
Instead of this please add: -'qemuxmlconftest' cases for the XML definition parser and commandline formatter -'virstoragetest' cases for the qcow2 metadata parser -'qemuxmlactivetest' cases for showing that the status XML will remember the node names of the data file
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
On Sat, Sep 07, 2024 at 17:15:15 +0300, Nikolai Barybin via Devel wrote: As I've fixed merge conflicts in this series in order to review it you can fetch the fixed patches at: git fetch https://gitlab.com/pipo.sk/libvirt.git review-qcow2-datafile
participants (2)
-
Nikolai Barybin -
Peter Krempa