On Wed, Nov 20, 2024 at 18:48:39 +0300, Nikolai Barybin via Devel wrote:
> Data files are simple raw images. Thus, we don't need to parse
> too much. The main objectives are:
>
> - allow only RAW format
> - forbid storage slices
> - include this parsing/formatting into backing chain parse/format as
> well as into top storage source parse/format because data file can
> belong to backing image
>
> Signed-off-by: Nikolai Barybin <nikolai.barybin@xxxxxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 100 +++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 13 ++++++
> 2 files changed, 113 insertions(+)
As I've already mentioned before I think that it'll make much more sense
if the element is called <dataStore> (as it doesn't necessarily have to
be a file) and also that it's nested under:
<disk ...>
<source file=...>
<dataStore type='file'>
<source ...>
</dataStore>
</source>
</disk>
The data-file qcow2 feature ties the data very closely to the source so
I don't think the same treatment as we have with <backingStore> would
model this relationship correctly.
In fact I consider that <backingStore> should have been also nested as
above.
I'll do the appropriate adjustment.
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 295707ec1f..9348c12725 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7670,6 +7670,62 @@ 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 = virXMLPropStringRequired(ctxt->node, "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 (virStorageFileFormatTypeFromString(format) != VIR_STORAGE_FILE_RAW) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("unexpected disk data file format '%1$s', only raw format is supported"),
> + format);
> + return -1;
> + }
> +
> + if (!(dataFileStore = virDomainStorageSourceParseBase(type, format, NULL)))
> + return -1;
> +
> + if (virDomainStorageSourceParse(source, ctxt, dataFileStore, flags, xmlopt) < 0)
> + return -1;
> +
> + if (dataFileStore->sliceStorage) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("slices are not supported for fata file store source"));
> + return -1;
> + }
Validation should be done in src/conf/domain_validate.c or the
hypervisor-driver specific code. I'll move this there.
This is also lacking validation that the disk source is QCOW2 as no
other source supports this. I'll add that there as well, as move the
format check above.
[...]
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index a187ab4083..4d1939aa1b 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3909,6 +3909,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,
> @@ -4426,6 +4432,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,
Please do not unnecessarily export functions. Neither of these is being
used outside of the 'domain_conf.c' module.
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
