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@xxxxxxxxxxxxx>
> ---
> 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,
