Split out parts of the config parsing code to make
the parent function easier to read.
Signed-off-by: Ján Tomko <jtomko@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)
+{
+ char *user = NULL, *group = NULL;
+ char **controllers = NULL;
+ char **namespaces = NULL;
+ 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;
+ 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;
+
+ if (virConfGetValueStringList(conf, "cgroup_controllers", false,
+ &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,
+ &cfg->cgroupDeviceACL) < 0)
+ goto cleanup;
+
+ if (virConfGetValueInt(conf, "seccomp_sandbox", &cfg->seccompSandbox) < 0)
+ goto cleanup;
+
+ if (virConfGetValueStringList(conf, "namespaces", false, &namespaces) < 0)
+ goto cleanup;
+
+ if (namespaces) {
+ virBitmapClearAll(cfg->namespaces);
+
+ for (i = 0; namespaces[i]; i++) {
+ int ns = qemuDomainNamespaceTypeFromString(namespaces[i]);
+
+ if (ns < 0) {
+ virReportError(VIR_ERR_CONF_SYNTAX,
+ _("Unknown namespace: %s"),
+ namespaces[i]);
+ goto cleanup;
+ }
+
+ if (!privileged) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("cannot use namespaces in session mode"));
+ goto cleanup;
+ }
+
+ if (!qemuDomainNamespaceAvailable(ns)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("%s namespace is not available"),
+ namespaces[i]);
+ goto cleanup;
+ }
+
+ if (virBitmapSetBit(cfg->namespaces, ns) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to enable namespace: %s"),
+ namespaces[i]);
+ goto cleanup;
+ }
+ }
+ }
+
+ ret = 0;
+ cleanup:
+ virStringListFree(controllers);
+ virStringListFree(namespaces);
+ VIR_FREE(user);
+ VIR_FREE(group);
+ return ret;
+}
+
static int
virQEMUDriverConfigLoadMemoryEntry(virQEMUDriverConfigPtr cfg,
virConfPtr conf)
@@ -464,14 +579,11 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
virConfPtr conf = NULL;
int ret = -1;
int rv;
- size_t i, j;
+ size_t i;
char *stdioHandler = NULL;
- char *user = NULL, *group = NULL;
- char **controllers = NULL;
char **hugetlbfs = NULL;
char **nvram = NULL;
char *corestr = NULL;
- char **namespaces = NULL;
bool tmp;
/* Just check the file is readable before opening it, otherwise
@@ -518,26 +630,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
goto cleanup;
- 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;
-
if (virConfGetValueBool(conf, "spice_tls", &cfg->spiceTLS) < 0)
goto cleanup;
if (virConfGetValueString(conf, "spice_tls_x509_cert_dir", &cfg->spiceTLSx509certdir) < 0)
@@ -667,41 +759,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
goto cleanup;
}
- 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;
-
- if (virConfGetValueStringList(conf, "cgroup_controllers", false,
- &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,
- &cfg->cgroupDeviceACL) < 0)
- goto cleanup;
-
if (virConfGetValueString(conf, "save_image_format", &cfg->saveImageFormat) < 0)
goto cleanup;
if (virConfGetValueString(conf, "dump_image_format", &cfg->dumpImageFormat) < 0)
@@ -812,9 +869,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
if (virConfGetValueUInt(conf, "keepalive_count", &cfg->keepAliveCount) < 0)
goto cleanup;
- if (virConfGetValueInt(conf, "seccomp_sandbox", &cfg->seccompSandbox) < 0)
- goto cleanup;
-
if (virConfGetValueString(conf, "migration_host", &cfg->migrateHost) < 0)
goto cleanup;
virStringStripIPv6Brackets(cfg->migrateHost);
@@ -863,44 +917,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
if (virConfGetValueUInt(conf, "gluster_debug_level", &cfg->glusterDebugLevel) < 0)
goto cleanup;
- if (virConfGetValueStringList(conf, "namespaces", false, &namespaces) < 0)
+ if (virQEMUDriverConfigLoadSecurityEntry(cfg, conf, privileged) < 0)
goto cleanup;
- if (namespaces) {
- virBitmapClearAll(cfg->namespaces);
-
- for (i = 0; namespaces[i]; i++) {
- int ns = qemuDomainNamespaceTypeFromString(namespaces[i]);
-
- if (ns < 0) {
- virReportError(VIR_ERR_CONF_SYNTAX,
- _("Unknown namespace: %s"),
- namespaces[i]);
- goto cleanup;
- }
-
- if (!privileged) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("cannot use namespaces in session mode"));
- goto cleanup;
- }
-
- if (!qemuDomainNamespaceAvailable(ns)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("%s namespace is not available"),
- namespaces[i]);
- goto cleanup;
- }
-
- if (virBitmapSetBit(cfg->namespaces, ns) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to enable namespace: %s"),
- namespaces[i]);
- goto cleanup;
- }
- }
- }
-
if (virQEMUDriverConfigLoadMemoryEntry(cfg, conf) < 0)
goto cleanup;
@@ -910,13 +929,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
ret = 0;
cleanup:
- virStringListFree(namespaces);
- virStringListFree(controllers);
virStringListFree(hugetlbfs);
virStringListFree(nvram);
VIR_FREE(corestr);
- VIR_FREE(user);
- VIR_FREE(group);
virConfFree(conf);
return ret;
}
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@redhat.com>
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jan 17, 2019 at 08:21:00AM -0500, John Ferlan wrote: > > >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@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... > The division is based on src/qemu/libvirtd_qemu.aug [...] >> +> + if (virConfGetValueInt(conf, "seccomp_sandbox", >&cfg->seccompSandbox) < 0) >> + goto cleanup; >> + > >And again, not security related. > How is seccomp not security related? Jano -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 1/18/19 7:03 AM, Ján Tomko wrote: > On Thu, Jan 17, 2019 at 08:21:00AM -0500, John Ferlan wrote: >> >> >> 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@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... >> > > The division is based on src/qemu/libvirtd_qemu.aug > > [...] > >>> +> + if (virConfGetValueInt(conf, "seccomp_sandbox", >> &cfg->seccompSandbox) < 0) >>> + goto cleanup; >>> + >> >> And again, not security related. >> > > How is seccomp not security related? > > Jano Bad cut/snip by me - I meant after seccomp, as in the namespace stuff. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.