Re: [PATCH 1/4] conf: introduce support for multiple ACPI tables

[Michal Prívozník <mprivozn@xxxxxxxxxx>]

On 2/18/25 19:12, Daniel P. Berrangé wrote:
> Currently we parse
> 
>    <os>
>      <acpi>
>        <table type="slic">...path...</table>
>      </acpi>
>    </os>
> 
> into a flat 'char *slic_table' field which is rather an anti-pattern
> as it has special cased a single attribute type.
> 
> This rewrites the internal design to permit multiple table types to
> be parsed, should we add more in future. Each type is permitted to
> only appear once.
> 
> The Xen code is fairly dubious in its use of 'slic_table' to hold
> Xen's 'acpi_firmware' config option, as IIUC Xen's config is not
> limited to accepting a single table per file. It takes a concatenation
> of all data and ought to be represented as such. This is left for a
> future contributor to solve.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> ---
>  src/conf/domain_conf.c          | 87 +++++++++++++++++++++++++--------
>  src/conf/domain_conf.h          | 21 +++++++-
>  src/libxl/libxl_conf.c          |  8 ++-
>  src/libxl/xen_xl.c              | 22 +++++++--
>  src/qemu/qemu_command.c         | 13 +++--
>  src/security/security_dac.c     | 18 ++++---
>  src/security/security_selinux.c | 16 +++---
>  src/security/virt-aa-helper.c   |  5 +-
>  8 files changed, 143 insertions(+), 47 deletions(-)


Please consider squashing in the following:

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 04fb893587..69e9255494 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -17888,29 +17888,30 @@ virDomainDefParseBootAcpiOptions(virDomainDef *def,
 {
     int n;
     g_autofree xmlNodePtr *nodes = NULL;
-    g_autofree char *tmp = NULL;
     size_t ntables = 0;
     virDomainOSACPITableDef **tables = NULL;
     size_t i;
-    size_t j;
 
     if ((n = virXPathNodeSet("./os/acpi/table", ctxt, &nodes)) < 0)
         return -1;
 
+    if (n == 0)
+        return 0;
+
+    tables = g_new0(virDomainOSACPITableDef *, n);
+
     for (i = 0; i < n; i++) {
-        int type;
-        tmp = virXMLPropString(nodes[i], "type");
+        g_autofree char *path = virXMLNodeContentString(nodes[i]);
+        virDomainOsACPITable type;
+        size_t j;
 
-        if (!tmp) {
-            virReportError(VIR_ERR_XML_ERROR, "%s",
-                           _("Missing acpi table type"));
+        if (!path)
             goto error;
-        }
 
-        if ((type = virDomainOsACPITableTypeFromString(tmp)) < 0) {
-            virReportError(VIR_ERR_XML_ERROR,
-                           _("Unknown acpi table type: %1$s"),
-                           tmp);
+        if (virXMLPropEnum(nodes[i], "type",
+                           virDomainOsACPITableTypeFromString,
+                           VIR_XML_PROP_REQUIRED,
+                           &type) < 0) {
             goto error;
         }
 
@@ -17918,22 +17919,15 @@ virDomainDefParseBootAcpiOptions(virDomainDef *def,
             if (tables[j]->type == type) {
                 virReportError(VIR_ERR_XML_ERROR,
                                _("ACPI table type '%1$s' may only appear once"),
-                               tmp);
+                               virDomainOsACPITableTypeToString(type));
                 goto error;
             }
         }
 
-        VIR_FREE(tmp);
-        if (!(tmp = virXMLNodeContentString(nodes[i])))
-            goto error;
-
-        tables = g_renew(virDomainOSACPITableDef *, tables, ntables + 1);
         tables[ntables] = g_new0(virDomainOSACPITableDef, 1);
         tables[ntables]->type = type;
-        tables[ntables]->path = virFileSanitizePath(tmp);
+        tables[ntables]->path = virFileSanitizePath(path);
         ntables++;
-
-        VIR_FREE(tmp);
     }
 
     def->os.nacpiTables = ntables;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 7735cce325..e5b0c42171 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2483,7 +2483,7 @@ typedef enum {
 VIR_ENUM_DECL(virDomainOsACPITable);
 
 struct _virDomainOSACPITableDef {
-    int type;
+    virDomainOsACPITable type;
     char *path;
 };
 
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 30a9f806f0..db8c29ec1d 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -611,6 +611,8 @@ virDomainObjTaint;
 virDomainObjUpdateModificationImpact;
 virDomainObjWait;
 virDomainObjWaitUntil;
+virDomainOsACPITableTypeFromString;
+virDomainOsACPITableTypeToString;
 virDomainOsDefFirmwareTypeFromString;
 virDomainOsDefFirmwareTypeToString;
 virDomainOSTypeFromString;
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 939478a625..23de0be9db 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -974,7 +974,7 @@ get_files(vahControl * ctl)
         if (vah_add_file(&buf, ctl->def->os.dtb, "r") != 0)
             goto cleanup;
 
-    for (i = 0; i < def->os.nacpiTables; i++) {
+    for (i = 0; i < ctl->def->os.nacpiTables; i++) {
         if (vah_add_file(&buf, ctl->def->os.acpiTables[i]->path, "r") != 0)
             goto cleanup;
     }


Michal