[PATCH v2 12/13] conf: implement XML parsing/formatingo for dataFileStore

Nikolai Barybin via Devel posted 13 patches 1 year, 5 months ago
There is a newer version of this series
[PATCH v2 12/13] conf: implement XML parsing/formatingo for dataFileStore
Posted by Nikolai Barybin via Devel 1 year, 5 months ago
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
Re: [PATCH v2 12/13] conf: implement XML parsing/formatingo for dataFileStore
Posted by Peter Krempa 1 year, 3 months ago
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,