12:12 p.m.
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(a)redhat.com>
---
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(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 49555efc56..04fb893587 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1457,6 +1457,11 @@ VIR_ENUM_IMPL(virDomainOsDefFirmwareFeature,
"secure-boot",
);
+VIR_ENUM_IMPL(virDomainOsACPITable,
+ VIR_DOMAIN_OS_ACPI_TABLE_TYPE_LAST,
+ "slic",
+);
+
VIR_ENUM_IMPL(virDomainCFPC,
VIR_DOMAIN_CFPC_LAST,
"none",
@@ -3899,6 +3904,15 @@ virDomainSecDefFree(virDomainSecDef *def)
g_free(def);
}
+void virDomainOSACPITableDefFree(virDomainOSACPITableDef *def)
+{
+ if (!def)
+ return;
+ g_free(def->path);
+ g_free(def);
+}
+
+
static void
virDomainOSDefClear(virDomainOSDef *os)
{
@@ -3924,7 +3938,9 @@ virDomainOSDefClear(virDomainOSDef *os)
g_free(os->cmdline);
g_free(os->dtb);
g_free(os->root);
- g_free(os->slic_table);
+ for (i = 0; i < os->nacpiTables; i++)
+ virDomainOSACPITableDefFree(os->acpiTables[i]);
+ g_free(os->acpiTables);
virDomainLoaderDefFree(os->loader);
g_free(os->bootloader);
g_free(os->bootloaderArgs);
@@ -17873,40 +17889,64 @@ 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 > 1) {
- virReportError(VIR_ERR_XML_ERROR, "%s",
- _("Only one acpi table is supported"));
- return -1;
- }
-
- if (n == 1) {
- tmp = virXMLPropString(nodes[0], "type");
+ for (i = 0; i < n; i++) {
+ int type;
+ tmp = virXMLPropString(nodes[i], "type");
if (!tmp) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("Missing acpi table type"));
- return -1;
+ goto error;
}
- if (STREQ_NULLABLE(tmp, "slic")) {
- VIR_FREE(tmp);
- if (!(tmp = virXMLNodeContentString(nodes[0])))
- return -1;
-
- def->os.slic_table = virFileSanitizePath(tmp);
- } else {
+ if ((type = virDomainOsACPITableTypeFromString(tmp)) < 0) {
virReportError(VIR_ERR_XML_ERROR,
_("Unknown acpi table type: %1$s"),
tmp);
- return -1;
+ goto error;
}
+
+ for (j = 0; j < i; j++) {
+ if (tables[j]->type == type) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("ACPI table type '%1$s' may only appear
once"),
+ tmp);
+ 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);
+ ntables++;
+
+ VIR_FREE(tmp);
}
+ def->os.nacpiTables = ntables;
+ def->os.acpiTables = tables;
+
return 0;
+
+ error:
+ for (i = 0; i < ntables; i++) {
+ virDomainOSACPITableDefFree(tables[i]);
+ }
+ g_free(tables);
+ return -1;
}
@@ -28490,11 +28530,16 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def,
def->os.dtb);
virBufferEscapeString(buf, "<root>%s</root>\n",
def->os.root);
- if (def->os.slic_table) {
+
+ if (def->os.nacpiTables) {
virBufferAddLit(buf, "<acpi>\n");
virBufferAdjustIndent(buf, 2);
- virBufferEscapeString(buf, "<table
type='slic'>%s</table>\n",
- def->os.slic_table);
+ for (i = 0; i < def->os.nacpiTables; i++) {
+ virBufferAsprintf(buf, "<table type='%s'>",
+
virDomainOsACPITableTypeToString(def->os.acpiTables[i]->type));
+ virBufferEscapeString(buf, "%s</table>\n",
+ def->os.acpiTables[i]->path);
+ }
virBufferAdjustIndent(buf, -2);
virBufferAddLit(buf, "</acpi>\n");
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9da6586e66..7735cce325 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2474,6 +2474,24 @@ typedef enum {
VIR_ENUM_DECL(virDomainOsDefFirmwareFeature);
+typedef enum {
+ VIR_DOMAIN_OS_ACPI_TABLE_TYPE_SLIC,
+
+ VIR_DOMAIN_OS_ACPI_TABLE_TYPE_LAST
+} virDomainOsACPITable;
+
+VIR_ENUM_DECL(virDomainOsACPITable);
+
+struct _virDomainOSACPITableDef {
+ int type;
+ char *path;
+};
+
+typedef struct _virDomainOSACPITableDef virDomainOSACPITableDef;
+void virDomainOSACPITableDefFree(virDomainOSACPITableDef *def);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainOSACPITableDef, virDomainOSACPITableDefFree);
+
+
struct _virDomainOSDef {
int type;
virDomainOsDefFirmware firmware;
@@ -2496,7 +2514,8 @@ struct _virDomainOSDef {
char *cmdline;
char *dtb;
char *root;
- char *slic_table;
+ size_t nacpiTables;
+ virDomainOSACPITableDef **acpiTables;
virDomainLoaderDef *loader;
char *bootloader;
char *bootloaderArgs;
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index c404226e43..7fa1decd67 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -583,7 +583,13 @@ libxlMakeDomBuildInfo(virDomainDef *def,
#endif
/* copy SLIC table path to acpi_firmware */
- b_info->u.hvm.acpi_firmware = g_strdup(def->os.slic_table);
+ for (i = 0; i < def->os.nacpiTables; i++) {
+ if (def->os.acpiTables[i]->type != VIR_DOMAIN_OS_ACPI_TABLE_TYPE_SLIC)
+ continue;
+
+ b_info->u.hvm.acpi_firmware =
g_strdup(def->os.acpiTables[i]->path);
+ break;
+ }
if (def->nsounds > 0) {
/*
diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c
index 53f6871efc..a9f41f9ee2 100644
--- a/src/libxl/xen_xl.c
+++ b/src/libxl/xen_xl.c
@@ -106,6 +106,7 @@ xenParseXLOS(virConf *conf, virDomainDef *def, virCaps *caps)
g_autofree char *bios = NULL;
g_autofree char *bios_path = NULL;
g_autofree char *boot = NULL;
+ g_autofree char *slic = NULL;
int val = 0;
if (xenConfigGetString(conf, "bios", &bios, NULL) < 0)
@@ -133,8 +134,15 @@ xenParseXLOS(virConf *conf, virDomainDef *def, virCaps *caps)
}
}
- if (xenConfigCopyStringOpt(conf, "acpi_firmware",
&def->os.slic_table) < 0)
+ if (xenConfigCopyStringOpt(conf, "acpi_firmware", &slic) < 0)
return -1;
+ if (slic != NULL) {
+ def->os.nacpiTables = 1;
+ def->os.acpiTables = g_new0(virDomainOSACPITableDef *, 1);
+ def->os.acpiTables[0] = g_new0(virDomainOSACPITableDef, 1);
+ def->os.acpiTables[0]->type = VIR_DOMAIN_OS_ACPI_TABLE_TYPE_SLIC;
+ def->os.acpiTables[0]->path = g_steal_pointer(&slic);
+ }
if (xenConfigCopyStringOpt(conf, "kernel", &def->os.kernel) <
0)
return -1;
@@ -1134,9 +1142,15 @@ xenFormatXLOS(virConf *conf, virDomainDef *def)
return -1;
}
- if (def->os.slic_table &&
- xenConfigSetString(conf, "acpi_firmware", def->os.slic_table)
< 0)
- return -1;
+ for (i = 0; i < def->os.nacpiTables; i++) {
+ if (def->os.acpiTables[i]->type != VIR_DOMAIN_OS_ACPI_TABLE_TYPE_SLIC)
+ continue;
+
+ if (xenConfigSetString(conf, "acpi_firmware",
+ def->os.acpiTables[i]->path) < 0)
+ return -1;
+ break;
+ }
if (def->os.kernel &&
xenConfigSetString(conf, "kernel", def->os.kernel) < 0)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 54130ac4f0..1153d8e095 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -127,6 +127,11 @@ VIR_ENUM_IMPL(qemuNumaPolicy,
"restrictive",
);
+VIR_ENUM_DECL(qemuACPITableSIG);
+VIR_ENUM_IMPL(qemuACPITableSIG,
+ VIR_DOMAIN_OS_ACPI_TABLE_TYPE_LAST,
+ "SLIC");
+
const char *
qemuAudioDriverTypeToString(virDomainAudioType type)
@@ -5995,6 +6000,7 @@ qemuBuildBootCommandLine(virCommand *cmd,
{
g_auto(virBuffer) boot_buf = VIR_BUFFER_INITIALIZER;
g_autofree char *boot_opts_str = NULL;
+ size_t i;
if (def->os.bootmenu) {
if (def->os.bootmenu == VIR_TRISTATE_BOOL_YES)
@@ -6028,11 +6034,12 @@ qemuBuildBootCommandLine(virCommand *cmd,
virCommandAddArgList(cmd, "-append", def->os.cmdline, NULL);
if (def->os.dtb)
virCommandAddArgList(cmd, "-dtb", def->os.dtb, NULL);
- if (def->os.slic_table) {
+ for (i = 0; i < def->os.nacpiTables; i++) {
g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
virCommandAddArg(cmd, "-acpitable");
- virBufferAddLit(&buf, "sig=SLIC,file=");
- virQEMUBuildBufferEscapeComma(&buf, def->os.slic_table);
+ virBufferAsprintf(&buf, "sig=%s,file=",
+
qemuACPITableSIGTypeToString(def->os.acpiTables[i]->type));
+ virQEMUBuildBufferEscapeComma(&buf, def->os.acpiTables[i]->path);
virCommandAddArgBuffer(cmd, &buf);
}
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 0505f4e4a3..b4d61bc576 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -2050,9 +2050,10 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr,
virSecurityDACRestoreFileLabel(mgr, def->os.dtb) < 0)
rc = -1;
- if (def->os.slic_table &&
- virSecurityDACRestoreFileLabel(mgr, def->os.slic_table) < 0)
- rc = -1;
+ for (i = 0; i < def->os.nacpiTables; i++) {
+ if (virSecurityDACRestoreFileLabel(mgr, def->os.acpiTables[i]->path) <
0)
+ rc = -1;
+ }
if (def->pstore &&
virSecurityDACRestoreFileLabel(mgr, def->pstore->path) < 0)
@@ -2300,11 +2301,12 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr,
user, group, true) < 0)
return -1;
- if (def->os.slic_table &&
- virSecurityDACSetOwnership(mgr, NULL,
- def->os.slic_table,
- user, group, true) < 0)
- return -1;
+ for (i = 0; i < def->os.nacpiTables; i++) {
+ if (virSecurityDACSetOwnership(mgr, NULL,
+ def->os.acpiTables[i]->path,
+ user, group, true) < 0)
+ return -1;
+ }
if (def->pstore &&
virSecurityDACSetOwnership(mgr, NULL,
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index cdc32d9b34..b8659e33d6 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -3013,9 +3013,10 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr,
virSecuritySELinuxRestoreFileLabel(mgr, def->os.dtb, true) < 0)
rc = -1;
- if (def->os.slic_table &&
- virSecuritySELinuxRestoreFileLabel(mgr, def->os.slic_table, true) < 0)
- rc = -1;
+ for (i = 0; i < def->os.nacpiTables; i++) {
+ if (virSecuritySELinuxRestoreFileLabel(mgr, def->os.acpiTables[i]->path,
true) < 0)
+ rc = -1;
+ }
if (def->pstore &&
virSecuritySELinuxRestoreFileLabel(mgr, def->pstore->path, true) < 0)
@@ -3443,10 +3444,11 @@ virSecuritySELinuxSetAllLabel(virSecurityManager *mgr,
data->content_context, true) < 0)
return -1;
- if (def->os.slic_table &&
- virSecuritySELinuxSetFilecon(mgr, def->os.slic_table,
- data->content_context, true) < 0)
- return -1;
+ for (i = 0; i < def->os.nacpiTables; i++) {
+ if (virSecuritySELinuxSetFilecon(mgr, def->os.acpiTables[i]->path,
+ data->content_context, true) < 0)
+ return -1;
+ }
if (def->pstore &&
virSecuritySELinuxSetFilecon(mgr, def->pstore->path,
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 1626d5a89c..939478a625 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -974,9 +974,10 @@ get_files(vahControl * ctl)
if (vah_add_file(&buf, ctl->def->os.dtb, "r") != 0)
goto cleanup;
- if (ctl->def->os.slic_table)
- if (vah_add_file(&buf, ctl->def->os.slic_table, "r") != 0)
+ for (i = 0; i < def->os.nacpiTables; i++) {
+ if (vah_add_file(&buf, ctl->def->os.acpiTables[i]->path,
"r") != 0)
goto cleanup;
+ }
if (ctl->def->pstore)
if (vah_add_file(&buf, ctl->def->pstore->path, "rw") !=
0)
--
2.47.1