Re: [libvirt] [PATCH 07/23] qemu_conf: split out virQEMUDriverConfigLoadSecurityEntry
On 1/15/19 8:23 AM, Ján Tomko wrote:
Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
---
src/qemu/qemu_conf.c | 219 +++++++++++++++++++++++--------------------
1 file changed, 117 insertions(+), 102 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 7fdfed7db1..135cb9e25d 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -423,6 +423,121 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs,
}
+static int
+virQEMUDriverConfigLoadSecurityEntry(virQEMUDriverConfigPtr cfg,
+ virConfPtr conf,
+ bool privileged)
This does security, cgroups, and namespaces...
+{
+ char *user = NULL, *group = NULL;
VIR_AUTOPTR(char *)
+ char **controllers = NULL;
+ char **namespaces = NULL;
VIR_AUTOPTR(virString)
+ int ret = -1;
+ size_t i, j;
+
+ if (virConfGetValueStringList(conf, "security_driver", true,
&cfg->securityDriverNames) < 0)
+ goto cleanup;
+
+ for (i = 0; cfg->securityDriverNames && cfg->securityDriverNames[i] !=
NULL; i++) {
+ for (j = i + 1; cfg->securityDriverNames[j] != NULL; j++) {
+ if (STREQ(cfg->securityDriverNames[i],
+ cfg->securityDriverNames[j])) {
+ virReportError(VIR_ERR_CONF_SYNTAX,
+ _("Duplicate security driver %s"),
+ cfg->securityDriverNames[i]);
+ goto cleanup;
+ }
+ }
+ }
+
+ if (virConfGetValueBool(conf, "security_default_confined",
&cfg->securityDefaultConfined) < 0)
+ goto cleanup;
+ if (virConfGetValueBool(conf, "security_require_confined",
&cfg->securityRequireConfined) < 0)
+ goto cleanup;
nit: blank line for readability/separation
+ if (virConfGetValueString(conf, "user", &user)
< 0)
+ goto cleanup;
+ if (user && virGetUserID(user, &cfg->user) < 0)
+ goto cleanup;
+
+ if (virConfGetValueString(conf, "group", &group) < 0)
+ goto cleanup;
+ if (group && virGetGroupID(group, &cfg->group) < 0)
+ goto cleanup;
+
+ if (virConfGetValueBool(conf, "dynamic_ownership",
&cfg->dynamicOwnership) < 0)
+ goto cleanup;
+
The following hunk feels separable since it's not security related...
+ if (virConfGetValueStringList(conf,
"cgroup_controllers", false,
^^
Oh look an extra space (existing)... may as well fix it, but a separate
patch is not needed.
+ &controllers) < 0)
+ goto cleanup;
+
+ if (controllers) {
+ cfg-> cgroupControllers = 0;
+ for (i = 0; controllers[i] != NULL; i++) {
+ int ctl;
+ if ((ctl = virCgroupControllerTypeFromString(controllers[i])) < 0) {
+ virReportError(VIR_ERR_CONF_SYNTAX,
+ _("Unknown cgroup controller '%s'"),
+ controllers[i]);
+ goto cleanup;
+ }
+ cfg->cgroupControllers |= (1 << ctl);
+ }
+ }
+
+ if (virConfGetValueStringList(conf, "cgroup_device_acl", false,
^^
and again copy-paste probably
+ &cfg->cgroupDeviceACL) <
0)
+ goto cleanup;
end cgroup separable hunk
+> + if (virConfGetValueInt(conf, "seccomp_sandbox",
&cfg->seccompSandbox) < 0)
+ goto cleanup;
+
And again, not security related.
Still, for the concept of splitting it's fine. I trust you can
manipulate a bit more for a final result, so
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
[...]