[libvirt] [PATCH 00/23] Clean up qemu.conf parsing
Remove the odd three-phase defaults filling for TLS cert dirs and split out the huge function into smaller ones based on the entry groups in the augeas file. Ján Tomko (23): qemu: fix double space in augeas file qemu: group swtpm entry in augeas file qemu.conf: fill out TLS verify attributes after parsing qemu: fill out usage-specific TLS settings after parsing qemu_conf: split out virQEMUDriverConfigLoadSWTPMEntry qemu_conf: split out virQEMUDriverConfigLoadMemoryEntry qemu_conf: split out virQEMUDriverConfigLoadSecurityEntry qemu_conf: split out virQEMUDriverConfigLoadGlusterDebugEntry qemu_conf: split out virQEMUDriverConfigLoadNVRAMEntry qemu_conf: split out virQEMUDriverConfigLoadLogEntry qemu_conf: split out virQEMUDriverConfigLoadNetworkEntry qemu_conf: split out virQEMUDriverConfigLoadRPCEntry qemu_conf: split out virQEMUDriverConfigLoadDeviceEntry qemu_conf: split out virQEMUDriverConfigLoadProcessEntry qemu_conf: split out virQEMUDriverConfigLoadSaveEntry qemu_conf: split out virQEMUDriverConfigLoadRemoteDisplayEntry qemu_conf: split out virQEMUDriverConfigLoadSpecificTLS qemu_conf: split out virQEMUDriverConfigLoadSPICEEntry qemu_conf: split out virQEMUDriverConfigLoadNographicsEntry qemu_conf: split out virQEMUDriverConfigLoadVNCEntry qemu_conf: split out virQEMUDriverConfigLoadDefaultTLSEntry qemu_conf: rename checkdefaultTLSx509certdir qemu_conf: fix stray space src/qemu/libvirtd_qemu.aug | 9 +- src/qemu/qemu_conf.c | 751 +++++++++++++++++++++++-------------- src/qemu/qemu_conf.h | 8 +- src/qemu/qemu_driver.c | 3 + 4 files changed, 480 insertions(+), 291 deletions(-) -- 2.20.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/libvirtd_qemu.aug | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index ddc4bbfd1d..434ae510a0 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -76,7 +76,7 @@ module Libvirtd_qemu = | int_entry "seccomp_sandbox" | str_array_entry "namespaces" - let save_entry = str_entry "save_image_format" + let save_entry = str_entry "save_image_format" | str_entry "dump_image_format" | str_entry "snapshot_image_format" | str_entry "auto_dump_path" -- 2.20.1
On 1/15/19 8:22 AM, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/libvirtd_qemu.aug | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Trivial, Reviewed-by: John Ferlan <jferlan@redhat.com> John
They are meant to be together. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/libvirtd_qemu.aug | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index 434ae510a0..28bd851411 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -122,8 +122,8 @@ module Libvirtd_qemu = let nbd_entry = bool_entry "nbd_tls" | str_entry "nbd_tls_x509_cert_dir" - let swtpm_user_entry = str_entry "swtpm_user" - let swtpm_group_entry = str_entry "swtpm_group" + let swtpm_entry = str_entry "swtpm_user" + | str_entry "swtpm_group" (* Each entry in the config is one of the following ... *) let entry = default_tls_entry @@ -145,8 +145,7 @@ module Libvirtd_qemu = | memory_entry | vxhs_entry | nbd_entry - | swtpm_user_entry - | swtpm_group_entry + | swtpm_entry let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] let empty = [ label "#empty" . eol ] -- 2.20.1
On 1/15/19 8:22 AM, Ján Tomko wrote:
They are meant to be together.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/libvirtd_qemu.aug | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Yes, they are. Signed-off-by: John Ferlan <jferlan@redhat.com> John
Introduce a set of bool variables with the 'present' suffix to track whether the value was actually specified. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_conf.c | 28 ++++++++++++++++++++++++---- src/qemu/qemu_conf.h | 6 ++++++ src/qemu/qemu_driver.c | 3 +++ 3 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b03e38b831..a0855032e5 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -535,8 +535,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if ((rv = virConfGetValueBool(conf, "vnc_tls_x509_verify", &cfg->vncTLSx509verify)) < 0) goto cleanup; - if (rv == 0) - cfg->vncTLSx509verify = cfg->defaultTLSx509verify; + if (rv == 1) + cfg->vncTLSx509verifyPresent = true; if (virConfGetValueString(conf, "vnc_tls_x509_cert_dir", &cfg->vncTLSx509certdir) < 0) goto cleanup; if (virConfGetValueString(conf, "vnc_listen", &cfg->vncListen) < 0) @@ -601,8 +601,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if ((rv = virConfGetValueBool(conf, #val "_tls_x509_verify", \ &cfg->val## TLSx509verify)) < 0) \ goto cleanup; \ - if (rv == 0) \ - cfg->val## TLSx509verify = cfg->defaultTLSx509verify; \ + if (rv == 1) \ + cfg->val## TLSx509verifyPresent = true; \ if ((rv = virConfGetValueString(conf, #val "_tls_x509_cert_dir", \ &cfg->val## TLSx509certdir)) < 0) \ goto cleanup; \ @@ -1055,6 +1055,26 @@ virQEMUDriverConfigValidate(virQEMUDriverConfigPtr cfg) return 0; } +int +virQEMUDriverConfigSetDefaults(virQEMUDriverConfigPtr cfg) +{ + int ret = -1; + +#define SET_TLS_VERIFY_DEFAULT(val) \ + do { \ + if (!cfg->val## TLSx509verifyPresent) \ + cfg->val## TLSx509verify = cfg->defaultTLSx509verify; \ + } while (0) + + SET_TLS_VERIFY_DEFAULT(vnc); + SET_TLS_VERIFY_DEFAULT(chardev); + SET_TLS_VERIFY_DEFAULT(migrate); + +#undef SET_TLS_VERIFY_DEFAULT + + ret = 0; + return ret; +} virQEMUDriverConfigPtr virQEMUDriverGetConfig(virQEMUDriverPtr driver) { diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 1013cfcaed..87e730058b 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -122,6 +122,7 @@ struct _virQEMUDriverConfig { bool vncAutoUnixSocket; bool vncTLS; bool vncTLSx509verify; + bool vncTLSx509verifyPresent; bool vncSASL; char *vncTLSx509certdir; char *vncListen; @@ -139,10 +140,12 @@ struct _virQEMUDriverConfig { bool chardevTLS; char *chardevTLSx509certdir; bool chardevTLSx509verify; + bool chardevTLSx509verifyPresent; char *chardevTLSx509secretUUID; char *migrateTLSx509certdir; bool migrateTLSx509verify; + bool migrateTLSx509verifyPresent; char *migrateTLSx509secretUUID; unsigned int remotePortMin; @@ -317,6 +320,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, int virQEMUDriverConfigValidate(virQEMUDriverConfigPtr cfg); +int +virQEMUDriverConfigSetDefaults(virQEMUDriverConfigPtr cfg); + virQEMUDriverConfigPtr virQEMUDriverGetConfig(virQEMUDriverPtr driver); bool virQEMUDriverIsPrivileged(virQEMUDriverPtr driver); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1d961707cc..5032edec50 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -628,6 +628,9 @@ qemuStateInitialize(bool privileged, if (virQEMUDriverConfigValidate(cfg) < 0) goto error; + if (virQEMUDriverConfigSetDefaults(cfg) < 0) + goto error; + if (virFileMakePath(cfg->stateDir) < 0) { virReportSystemError(errno, _("Failed to create state dir %s"), cfg->stateDir); -- 2.20.1
On 1/15/19 8:22 AM, Ján Tomko wrote:
Introduce a set of bool variables with the 'present' suffix to track whether the value was actually specified.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_conf.c | 28 ++++++++++++++++++++++++---- src/qemu/qemu_conf.h | 6 ++++++ src/qemu/qemu_driver.c | 3 +++ 3 files changed, 33 insertions(+), 4 deletions(-)
6 of one, half-dozen of another. There should be two empty lines before/after the new method below even though it's not the norm in the file. And yes, the ret = 0; return ret; looks odd now, but I see usage in next patch... Reviewed-by: John Ferlan <jferlan@redhat.com> John
Instead of copying the default default values upfront and then wondering whether the user has given us a new default, leave the per-usage TLS certdirs and secrets empty during parsing and only fill them afterwards if they weren't provided by the user. This means that instead of looking whether the specific certdir paths match the default default, the Validate function (which is called inbetween parsing and setting the defaults) can error out for missing directories if the value is present, because it must've come from the user. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_conf.c | 150 ++++++++++++++++--------------------------- 1 file changed, 56 insertions(+), 94 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index a0855032e5..837bff6b30 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -277,35 +277,6 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) if (VIR_STRDUP(cfg->spiceListen, VIR_LOOPBACK_IPV4_ADDR) < 0) goto error; - /* - * If a "SYSCONFDIR" + "pki/libvirt-<val>" exists, then assume someone - * has created a val specific area to place service specific certificates. - * - * If the service specific directory doesn't exist, 'assume' that the - * user has created and populated the "SYSCONFDIR" + "pki/libvirt-default". - */ -#define SET_TLS_X509_CERT_DEFAULT(val) \ - do { \ - if (virFileExists(SYSCONFDIR "/pki/libvirt-"#val)) { \ - if (VIR_STRDUP(cfg->val ## TLSx509certdir, \ - SYSCONFDIR "/pki/libvirt-"#val) < 0) \ - goto error; \ - } else { \ - if (VIR_STRDUP(cfg->val ## TLSx509certdir, \ - cfg->defaultTLSx509certdir) < 0) \ - goto error; \ - } \ - } while (0) - - SET_TLS_X509_CERT_DEFAULT(vnc); - SET_TLS_X509_CERT_DEFAULT(spice); - SET_TLS_X509_CERT_DEFAULT(chardev); - SET_TLS_X509_CERT_DEFAULT(migrate); - SET_TLS_X509_CERT_DEFAULT(vxhs); - SET_TLS_X509_CERT_DEFAULT(nbd); - -#undef SET_TLS_X509_CERT_DEFAULT - cfg->remotePortMin = QEMU_REMOTE_PORT_MIN; cfg->remotePortMax = QEMU_REMOTE_PORT_MAX; @@ -452,45 +423,6 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } -/** - * @cfg: Just read config TLS values - * - * If the default_tls_x509_cert_dir was uncommented or changed from - * the default value assigned to the *_tls_x509_cert_dir values when - * virQEMUDriverConfigNew was executed, we need to check if we need - * to update the other defaults. - * - * Returns 0 on success, -1 on failure - */ -static int -virQEMUDriverConfigTLSDirResetDefaults(virQEMUDriverConfigPtr cfg) -{ - /* Not changed or set to the default default, nothing to do */ - if (!cfg->checkdefaultTLSx509certdir || - STREQ(cfg->defaultTLSx509certdir, SYSCONFDIR "/pki/qemu")) - return 0; - -#define CHECK_RESET_CERT_DIR_DEFAULT(val) \ - do { \ - if (STREQ(cfg->val ## TLSx509certdir, SYSCONFDIR "/pki/qemu")) { \ - VIR_FREE(cfg->val ## TLSx509certdir); \ - if (VIR_STRDUP(cfg->val ## TLSx509certdir, \ - cfg->defaultTLSx509certdir) < 0) \ - return -1; \ - } \ - } while (0) - - CHECK_RESET_CERT_DIR_DEFAULT(vnc); - CHECK_RESET_CERT_DIR_DEFAULT(spice); - CHECK_RESET_CERT_DIR_DEFAULT(chardev); - CHECK_RESET_CERT_DIR_DEFAULT(migrate); - CHECK_RESET_CERT_DIR_DEFAULT(vxhs); - CHECK_RESET_CERT_DIR_DEFAULT(nbd); - - return 0; -} - - int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, const char *filename, bool privileged) @@ -603,23 +535,13 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; \ if (rv == 1) \ cfg->val## TLSx509verifyPresent = true; \ - if ((rv = virConfGetValueString(conf, #val "_tls_x509_cert_dir", \ - &cfg->val## TLSx509certdir)) < 0) \ + if (virConfGetValueString(conf, #val "_tls_x509_cert_dir", \ + &cfg->val## TLSx509certdir) < 0) \ goto cleanup; \ if (virConfGetValueString(conf, \ #val "_tls_x509_secret_uuid", \ &cfg->val## TLSx509secretUUID) < 0) \ goto cleanup; \ - /* Only if a *tls_x509_cert_dir wasn't found (e.g. rv == 0), should \ - * we copy the defaultTLSx509secretUUID. If this environment needs \ - * a passphrase to decode the certificate, then it should provide \ - * it's own secretUUID for that. */ \ - if (rv == 0 && !cfg->val## TLSx509secretUUID && \ - cfg->defaultTLSx509secretUUID) { \ - if (VIR_STRDUP(cfg->val## TLSx509secretUUID, \ - cfg->defaultTLSx509secretUUID) < 0) \ - goto cleanup; \ - } \ } while (0) if (virConfGetValueBool(conf, "chardev_tls", &cfg->chardevTLS) < 0) @@ -630,9 +552,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, #undef GET_CONFIG_TLS_CERTINFO - if (virQEMUDriverConfigTLSDirResetDefaults(cfg) < 0) - goto cleanup; - if (virConfGetValueUInt(conf, "remote_websocket_port_min", &cfg->webSocketPortMin) < 0) goto cleanup; if (cfg->webSocketPortMin < QEMU_WEBSOCKET_PORT_MIN) { @@ -989,7 +908,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, int virQEMUDriverConfigValidate(virQEMUDriverConfigPtr cfg) { - /* If the default entry was uncommented, then validate existence */ if (cfg->checkdefaultTLSx509certdir) { if (!virFileExists(cfg->defaultTLSx509certdir)) { virReportError(VIR_ERR_CONF_SYNTAX, @@ -1000,11 +918,7 @@ virQEMUDriverConfigValidate(virQEMUDriverConfigPtr cfg) } } - /* For each of the others - if the value is not to the default default - * then check if the directory exists (this may duplicate the check done - * during virQEMUDriverConfigNew). - */ - if (STRNEQ(cfg->vncTLSx509certdir, SYSCONFDIR "/pki/qemu") && + if (cfg->vncTLSx509certdir && !virFileExists(cfg->vncTLSx509certdir)) { virReportError(VIR_ERR_CONF_SYNTAX, _("vnc_tls_x509_cert_dir directory '%s' does not exist"), @@ -1012,7 +926,7 @@ virQEMUDriverConfigValidate(virQEMUDriverConfigPtr cfg) return -1; } - if (STRNEQ(cfg->spiceTLSx509certdir, SYSCONFDIR "/pki/qemu") && + if (cfg->spiceTLSx509certdir && !virFileExists(cfg->spiceTLSx509certdir)) { virReportError(VIR_ERR_CONF_SYNTAX, _("spice_tls_x509_cert_dir directory '%s' does not exist"), @@ -1020,7 +934,7 @@ virQEMUDriverConfigValidate(virQEMUDriverConfigPtr cfg) return -1; } - if (STRNEQ(cfg->chardevTLSx509certdir, SYSCONFDIR "/pki/qemu") && + if (cfg->chardevTLSx509certdir && !virFileExists(cfg->chardevTLSx509certdir)) { virReportError(VIR_ERR_CONF_SYNTAX, _("chardev_tls_x509_cert_dir directory '%s' does not exist"), @@ -1028,7 +942,7 @@ virQEMUDriverConfigValidate(virQEMUDriverConfigPtr cfg) return -1; } - if (STRNEQ(cfg->migrateTLSx509certdir, SYSCONFDIR "/pki/qemu") && + if (cfg->migrateTLSx509certdir && !virFileExists(cfg->migrateTLSx509certdir)) { virReportError(VIR_ERR_CONF_SYNTAX, _("migrate_tls_x509_cert_dir directory '%s' does not exist"), @@ -1036,7 +950,7 @@ virQEMUDriverConfigValidate(virQEMUDriverConfigPtr cfg) return -1; } - if (STRNEQ(cfg->vxhsTLSx509certdir, SYSCONFDIR "/pki/qemu") && + if (cfg->vxhsTLSx509certdir && !virFileExists(cfg->vxhsTLSx509certdir)) { virReportError(VIR_ERR_CONF_SYNTAX, _("vxhs_tls_x509_cert_dir directory '%s' does not exist"), @@ -1044,7 +958,7 @@ virQEMUDriverConfigValidate(virQEMUDriverConfigPtr cfg) return -1; } - if (STRNEQ(cfg->nbdTLSx509certdir, SYSCONFDIR "/pki/qemu") && + if (cfg->nbdTLSx509certdir && !virFileExists(cfg->nbdTLSx509certdir)) { virReportError(VIR_ERR_CONF_SYNTAX, _("nbd_tls_x509_cert_dir directory '%s' does not exist"), @@ -1060,6 +974,53 @@ virQEMUDriverConfigSetDefaults(virQEMUDriverConfigPtr cfg) { int ret = -1; +#define SET_TLS_SECRET_UUID_DEFAULT(val) \ + do { \ + if (!cfg->val## TLSx509certdir && \ + !cfg->val## TLSx509secretUUID && \ + cfg->defaultTLSx509secretUUID) { \ + if (VIR_STRDUP(cfg->val## TLSx509secretUUID, \ + cfg->defaultTLSx509secretUUID) < 0) \ + goto cleanup; \ + } \ + } while (0) + + SET_TLS_SECRET_UUID_DEFAULT(chardev); + SET_TLS_SECRET_UUID_DEFAULT(migrate); + +#undef SET_TLS_SECRET_UUID_DEFAULT + + /* + * If a "SYSCONFDIR" + "pki/libvirt-<val>" exists, then assume someone + * has created a val specific area to place service specific certificates. + * + * If the service specific directory doesn't exist, 'assume' that the + * user has created and populated the "SYSCONFDIR" + "pki/libvirt-default". + */ +#define SET_TLS_X509_CERT_DEFAULT(val) \ + do { \ + if (cfg->val ## TLSx509certdir) \ + break; \ + if (virFileExists(SYSCONFDIR "/pki/libvirt-"#val)) { \ + if (VIR_STRDUP(cfg->val ## TLSx509certdir, \ + SYSCONFDIR "/pki/libvirt-"#val) < 0) \ + goto cleanup; \ + } else { \ + if (VIR_STRDUP(cfg->val ## TLSx509certdir, \ + cfg->defaultTLSx509certdir) < 0) \ + goto cleanup; \ + } \ + } while (0) + + SET_TLS_X509_CERT_DEFAULT(vnc); + SET_TLS_X509_CERT_DEFAULT(spice); + SET_TLS_X509_CERT_DEFAULT(chardev); + SET_TLS_X509_CERT_DEFAULT(migrate); + SET_TLS_X509_CERT_DEFAULT(vxhs); + SET_TLS_X509_CERT_DEFAULT(nbd); + +#undef SET_TLS_X509_CERT_DEFAULT + #define SET_TLS_VERIFY_DEFAULT(val) \ do { \ if (!cfg->val## TLSx509verifyPresent) \ @@ -1073,6 +1034,7 @@ virQEMUDriverConfigSetDefaults(virQEMUDriverConfigPtr cfg) #undef SET_TLS_VERIFY_DEFAULT ret = 0; + cleanup: return ret; } -- 2.20.1
On 1/15/19 8:23 AM, Ján Tomko wrote:
Instead of copying the default default values upfront and then wondering whether the user has given us a new default, leave the per-usage TLS certdirs and secrets empty during parsing and only fill them afterwards if they weren't provided by the user.
This means that instead of looking whether the specific certdir paths match the default default, the Validate function (which is called inbetween parsing and setting the defaults) can error
in between
out for missing directories if the value is present, because it must've come from the user.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_conf.c | 150 ++++++++++++++++--------------------------- 1 file changed, 56 insertions(+), 94 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
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 | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 837bff6b30..325e5ccfd5 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,30 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadSWTPMEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + char *swtpm_user = NULL, *swtpm_group = NULL; + int ret = -1; + + if (virConfGetValueString(conf, "swtpm_user", &swtpm_user) < 0) + goto cleanup; + if (swtpm_user && virGetUserID(swtpm_user, &cfg->swtpm_user) < 0) + goto cleanup; + + if (virConfGetValueString(conf, "swtpm_group", &swtpm_group) < 0) + goto cleanup; + if (swtpm_group && virGetGroupID(swtpm_group, &cfg->swtpm_group) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(swtpm_user); + VIR_FREE(swtpm_group); + return ret; +} + int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, const char *filename, bool privileged) @@ -433,7 +457,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, size_t i, j; char *stdioHandler = NULL; char *user = NULL, *group = NULL; - char *swtpm_user = NULL, *swtpm_group = NULL; char **controllers = NULL; char **hugetlbfs = NULL; char **nvram = NULL; @@ -871,14 +894,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueString(conf, "memory_backing_dir", &cfg->memoryBackingDir) < 0) goto cleanup; - if (virConfGetValueString(conf, "swtpm_user", &swtpm_user) < 0) - goto cleanup; - if (swtpm_user && virGetUserID(swtpm_user, &cfg->swtpm_user) < 0) - goto cleanup; - - if (virConfGetValueString(conf, "swtpm_group", &swtpm_group) < 0) - goto cleanup; - if (swtpm_group && virGetGroupID(swtpm_group, &cfg->swtpm_group) < 0) + if (virQEMUDriverConfigLoadSWTPMEntry(cfg, conf) < 0) goto cleanup; ret = 0; @@ -891,8 +907,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, VIR_FREE(corestr); VIR_FREE(user); VIR_FREE(group); - VIR_FREE(swtpm_user); - VIR_FREE(swtpm_group); virConfFree(conf); return ret; } -- 2.20.1
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 | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 837bff6b30..325e5ccfd5 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,30 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadSWTPMEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + char *swtpm_user = NULL, *swtpm_group = NULL;
Some would note these should be on separate lines, but I'll note that since we have it, these should be: VIR_AUTOPTR(char *) swtpm_user = NULL; VIR_AUTOPTR(char *) swtpm_group = NULL;
+ int ret = -1; + + if (virConfGetValueString(conf, "swtpm_user", &swtpm_user) < 0) + goto cleanup; + if (swtpm_user && virGetUserID(swtpm_user, &cfg->swtpm_user) < 0) + goto cleanup; + + if (virConfGetValueString(conf, "swtpm_group", &swtpm_group) < 0) + goto cleanup; + if (swtpm_group && virGetGroupID(swtpm_group, &cfg->swtpm_group) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(swtpm_user); + VIR_FREE(swtpm_group);
Thus freeing you of needing this and perhaps changing your logic above. I "understand" the "norm" is to just purely copy and then have another patch, but I really believe in this case it's so obvious that a separate patch isn't required especially since VIR_AUTOFREE is more commonly used.
+ return ret; +} +
one more blank line. If you feel you must create two patches out of this one patch, then go ahead, I trust that you can do that, but I think we really should use the VIR_AUTOPTR more now... BTW: I have a feeling VIR_AUTOPTR and 2 empty lines will become a recurring theme... Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
On Thu, Jan 17, 2019 at 08:03:44AM -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 | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 837bff6b30..325e5ccfd5 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,30 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadSWTPMEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + char *swtpm_user = NULL, *swtpm_group = NULL;
Some would note these should be on separate lines, but I'll note that since we have it, these should be:
VIR_AUTOPTR(char *) swtpm_user = NULL; VIR_AUTOPTR(char *) swtpm_group = NULL;
+ int ret = -1; + + if (virConfGetValueString(conf, "swtpm_user", &swtpm_user) < 0) + goto cleanup; + if (swtpm_user && virGetUserID(swtpm_user, &cfg->swtpm_user) < 0) + goto cleanup; + + if (virConfGetValueString(conf, "swtpm_group", &swtpm_group) < 0) + goto cleanup; + if (swtpm_group && virGetGroupID(swtpm_group, &cfg->swtpm_group) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(swtpm_user); + VIR_FREE(swtpm_group);
Thus freeing you of needing this and perhaps changing your logic above.
I "understand" the "norm" is to just purely copy and then have another patch, but I really believe in this case it's so obvious that a separate patch isn't required especially since VIR_AUTOFREE is more commonly used.
Yes, I'd rather not include any functional changes here (other than those necessary to deal with the (lack of a) cleanup section. Jano
On 1/18/19 6:56 AM, Ján Tomko wrote:
On Thu, Jan 17, 2019 at 08:03:44AM -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 | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 837bff6b30..325e5ccfd5 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,30 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadSWTPMEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + char *swtpm_user = NULL, *swtpm_group = NULL;
Some would note these should be on separate lines, but I'll note that since we have it, these should be:
VIR_AUTOPTR(char *) swtpm_user = NULL; VIR_AUTOPTR(char *) swtpm_group = NULL;
+ int ret = -1; + + if (virConfGetValueString(conf, "swtpm_user", &swtpm_user) < 0) + goto cleanup; + if (swtpm_user && virGetUserID(swtpm_user, &cfg->swtpm_user) < 0) + goto cleanup; + + if (virConfGetValueString(conf, "swtpm_group", &swtpm_group) < 0) + goto cleanup; + if (swtpm_group && virGetGroupID(swtpm_group, &cfg->swtpm_group) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(swtpm_user); + VIR_FREE(swtpm_group);
Thus freeing you of needing this and perhaps changing your logic above.
I "understand" the "norm" is to just purely copy and then have another patch, but I really believe in this case it's so obvious that a separate patch isn't required especially since VIR_AUTOFREE is more commonly used.
Yes, I'd rather not include any functional changes here (other than those necessary to deal with the (lack of a) cleanup section.
Jano
So I see you left the AUTOPTR/AUTOFREE to some future change... Is that in your short term plan or waiting for some bite size libvirt first task to complete? John
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 | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 325e5ccfd5..7fdfed7db1 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,16 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadMemoryEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + if (virConfGetValueString(conf, "memory_backing_dir", &cfg->memoryBackingDir) < 0) + return -1; + + return 0; +} + static int virQEMUDriverConfigLoadSWTPMEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) @@ -891,7 +901,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, } } - if (virConfGetValueString(conf, "memory_backing_dir", &cfg->memoryBackingDir) < 0) + if (virQEMUDriverConfigLoadMemoryEntry(cfg, conf) < 0) goto cleanup; if (virQEMUDriverConfigLoadSWTPMEntry(cfg, conf) < 0) -- 2.20.1
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 | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 325e5ccfd5..7fdfed7db1 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,16 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadMemoryEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + if (virConfGetValueString(conf, "memory_backing_dir", &cfg->memoryBackingDir) < 0) + return -1; + + return 0;
return virConfGetValueString(conf, "memory_backing_dir", &cfg->memoryBackingDir);
+} +
Add an extra blank line here... Reviewed-by: John Ferlan <jferlan@redhat.com> John Although this one's almost pointless, but for consistency, fine. [...]
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
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; +
+ if (virConfGetValueStringList(conf, "cgroup_controllers", false, ^^ Oh look an extra space (existing)... may as well fix it, but a separate
The following hunk feels separable since it's not security related... 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 [...]
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
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
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 | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 135cb9e25d..ca31a222e8 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,15 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadGlusterDebugEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + if (virConfGetValueUInt(conf, "gluster_debug_level", &cfg->glusterDebugLevel) < 0) + return -1; + return 0; +} + static int virQEMUDriverConfigLoadSecurityEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf, @@ -914,7 +923,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; } } - if (virConfGetValueUInt(conf, "gluster_debug_level", &cfg->glusterDebugLevel) < 0) + + if (virQEMUDriverConfigLoadGlusterDebugEntry(cfg, conf) < 0) goto cleanup; if (virQEMUDriverConfigLoadSecurityEntry(cfg, conf, privileged) < 0) -- 2.20.1
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 | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 135cb9e25d..ca31a222e8 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,15 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadGlusterDebugEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + if (virConfGetValueUInt(conf, "gluster_debug_level", &cfg->glusterDebugLevel) < 0) + return -1; + return 0;
return virConfGetValueUInt(...)
+} +
blank line Reviewed-by: John Ferlan <jferlan@redhat.com> John
static int virQEMUDriverConfigLoadSecurityEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf, @@ -914,7 +923,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; } } - if (virConfGetValueUInt(conf, "gluster_debug_level", &cfg->glusterDebugLevel) < 0) + + if (virQEMUDriverConfigLoadGlusterDebugEntry(cfg, conf) < 0) goto cleanup;
if (virQEMUDriverConfigLoadSecurityEntry(cfg, conf, privileged) < 0)
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 | 49 +++++++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ca31a222e8..29f948346d 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,37 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + char **nvram = NULL; + int ret = -1; + size_t i; + + if (virConfGetValueStringList(conf, "nvram", false, &nvram) < 0) + goto cleanup; + if (nvram) { + virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares); + + cfg->nfirmwares = virStringListLength((const char *const *)nvram); + if (nvram[0] && VIR_ALLOC_N(cfg->firmwares, cfg->nfirmwares) < 0) + goto cleanup; + + for (i = 0; nvram[i] != NULL; i++) { + if (VIR_ALLOC(cfg->firmwares[i]) < 0) + goto cleanup; + if (virFirmwareParse(nvram[i], cfg->firmwares[i]) < 0) + goto cleanup; + } + } + + ret = 0; + cleanup: + virStringListFree(nvram); + return ret; +} + static int virQEMUDriverConfigLoadGlusterDebugEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) @@ -591,7 +622,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, size_t i; char *stdioHandler = NULL; char **hugetlbfs = NULL; - char **nvram = NULL; char *corestr = NULL; bool tmp; @@ -907,22 +937,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueBool(conf, "log_timestamp", &cfg->logTimestamp) < 0) goto cleanup; - if (virConfGetValueStringList(conf, "nvram", false, &nvram) < 0) + if (virQEMUDriverConfigLoadNVRAMEntry(cfg, conf) < 0) goto cleanup; - if (nvram) { - virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares); - - cfg->nfirmwares = virStringListLength((const char *const *)nvram); - if (nvram[0] && VIR_ALLOC_N(cfg->firmwares, cfg->nfirmwares) < 0) - goto cleanup; - - for (i = 0; nvram[i] != NULL; i++) { - if (VIR_ALLOC(cfg->firmwares[i]) < 0) - goto cleanup; - if (virFirmwareParse(nvram[i], cfg->firmwares[i]) < 0) - goto cleanup; - } - } if (virQEMUDriverConfigLoadGlusterDebugEntry(cfg, conf) < 0) goto cleanup; @@ -940,7 +956,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, cleanup: virStringListFree(hugetlbfs); - virStringListFree(nvram); VIR_FREE(corestr); virConfFree(conf); return ret; -- 2.20.1
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 | 49 +++++++++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index ca31a222e8..29f948346d 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,37 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + char **nvram = NULL;
VIR_AUTOPTR(virString)
+ int ret = -1; + size_t i; + + if (virConfGetValueStringList(conf, "nvram", false, &nvram) < 0) + goto cleanup; + if (nvram) { + virFirmwareFreeList(cfg->firmwares, cfg->nfirmwares); + + cfg->nfirmwares = virStringListLength((const char *const *)nvram); + if (nvram[0] && VIR_ALLOC_N(cfg->firmwares, cfg->nfirmwares) < 0) + goto cleanup; + + for (i = 0; nvram[i] != NULL; i++) { + if (VIR_ALLOC(cfg->firmwares[i]) < 0) + goto cleanup; + if (virFirmwareParse(nvram[i], cfg->firmwares[i]) < 0) + goto cleanup; + } + } + + ret = 0; + cleanup: + virStringListFree(nvram); + return ret; +} +
blank line Reviewed-by: John Ferlan <jferlan@redhat.com> John
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 | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 29f948346d..b7d258f17a 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,16 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadLogEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + if (virConfGetValueBool(conf, "log_timestamp", &cfg->logTimestamp) < 0) + return -1; + + return 0; +} + static int virQEMUDriverConfigLoadNVRAMEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) @@ -934,7 +944,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; } - if (virConfGetValueBool(conf, "log_timestamp", &cfg->logTimestamp) < 0) + if (virQEMUDriverConfigLoadLogEntry(cfg, conf) < 0) goto cleanup; if (virQEMUDriverConfigLoadNVRAMEntry(cfg, conf) < 0) -- 2.20.1
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 | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 29f948346d..b7d258f17a 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,16 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadLogEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + if (virConfGetValueBool(conf, "log_timestamp", &cfg->logTimestamp) < 0) + return -1; + + return 0;
return virConfGetValueBool(...);
+} +
blank line Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
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 | 99 +++++++++++++++++++++++++------------------- 1 file changed, 56 insertions(+), 43 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b7d258f17a..8aa5157cd1 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,60 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadNetworkEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf, + const char *filename) +{ + if (virConfGetValueUInt(conf, "migration_port_min", &cfg->migrationPortMin) < 0) + return -1; + if (cfg->migrationPortMin <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: migration_port_min: port must be greater than 0"), + filename); + return -1; + } + + if (virConfGetValueUInt(conf, "migration_port_max", &cfg->migrationPortMax) < 0) + return -1; + if (cfg->migrationPortMax > 65535 || + cfg->migrationPortMax < cfg->migrationPortMin) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: migration_port_max: port must be between " + "the minimal port %d and 65535"), + filename, cfg->migrationPortMin); + return -1; + } + + if (virConfGetValueString(conf, "migration_host", &cfg->migrateHost) < 0) + return -1; + virStringStripIPv6Brackets(cfg->migrateHost); + if (cfg->migrateHost && + (STRPREFIX(cfg->migrateHost, "localhost") || + virSocketAddrIsNumericLocalhost(cfg->migrateHost))) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("migration_host must not be the address of" + " the local machine: %s"), + cfg->migrateHost); + return -1; + } + + if (virConfGetValueString(conf, "migration_address", &cfg->migrationAddress) < 0) + return -1; + virStringStripIPv6Brackets(cfg->migrationAddress); + if (cfg->migrationAddress && + (STRPREFIX(cfg->migrationAddress, "localhost") || + virSocketAddrIsNumericLocalhost(cfg->migrationAddress))) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("migration_address must not be the address of" + " the local machine: %s"), + cfg->migrationAddress); + return -1; + } + + return 0; +} + static int virQEMUDriverConfigLoadLogEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) @@ -788,25 +842,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; } - if (virConfGetValueUInt(conf, "migration_port_min", &cfg->migrationPortMin) < 0) - goto cleanup; - if (cfg->migrationPortMin <= 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s: migration_port_min: port must be greater than 0"), - filename); - goto cleanup; - } - - if (virConfGetValueUInt(conf, "migration_port_max", &cfg->migrationPortMax) < 0) - goto cleanup; - if (cfg->migrationPortMax > 65535 || - cfg->migrationPortMax < cfg->migrationPortMin) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s: migration_port_max: port must be between " - "the minimal port %d and 65535"), - filename, cfg->migrationPortMin); - goto cleanup; - } if (virConfGetValueString(conf, "save_image_format", &cfg->saveImageFormat) < 0) goto cleanup; @@ -858,6 +893,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (virConfGetValueBool(conf, "clear_emulator_capabilities", &cfg->clearEmulatorCapabilities) < 0) goto cleanup; + if ((rv = virConfGetValueBool(conf, "allow_disk_format_probing", &tmp)) < 0) goto cleanup; if (rv == 1 && tmp) { @@ -918,31 +954,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueUInt(conf, "keepalive_count", &cfg->keepAliveCount) < 0) goto cleanup; - if (virConfGetValueString(conf, "migration_host", &cfg->migrateHost) < 0) + if (virQEMUDriverConfigLoadNetworkEntry(cfg, conf, filename) < 0) goto cleanup; - virStringStripIPv6Brackets(cfg->migrateHost); - if (cfg->migrateHost && - (STRPREFIX(cfg->migrateHost, "localhost") || - virSocketAddrIsNumericLocalhost(cfg->migrateHost))) { - virReportError(VIR_ERR_CONF_SYNTAX, - _("migration_host must not be the address of" - " the local machine: %s"), - cfg->migrateHost); - goto cleanup; - } - - if (virConfGetValueString(conf, "migration_address", &cfg->migrationAddress) < 0) - goto cleanup; - virStringStripIPv6Brackets(cfg->migrationAddress); - if (cfg->migrationAddress && - (STRPREFIX(cfg->migrationAddress, "localhost") || - virSocketAddrIsNumericLocalhost(cfg->migrationAddress))) { - virReportError(VIR_ERR_CONF_SYNTAX, - _("migration_address must not be the address of" - " the local machine: %s"), - cfg->migrationAddress); - goto cleanup; - } if (virQEMUDriverConfigLoadLogEntry(cfg, conf) < 0) goto cleanup; -- 2.20.1
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 | 99 +++++++++++++++++++++++++------------------- 1 file changed, 56 insertions(+), 43 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b7d258f17a..8aa5157cd1 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,60 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadNetworkEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf, + const char *filename) +{ + if (virConfGetValueUInt(conf, "migration_port_min", &cfg->migrationPortMin) < 0) + return -1; + if (cfg->migrationPortMin <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: migration_port_min: port must be greater than 0"), + filename); + return -1; + } + + if (virConfGetValueUInt(conf, "migration_port_max", &cfg->migrationPortMax) < 0) + return -1; + if (cfg->migrationPortMax > 65535 || + cfg->migrationPortMax < cfg->migrationPortMin) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: migration_port_max: port must be between " + "the minimal port %d and 65535"), + filename, cfg->migrationPortMin); + return -1; + } + + if (virConfGetValueString(conf, "migration_host", &cfg->migrateHost) < 0) + return -1; + virStringStripIPv6Brackets(cfg->migrateHost); + if (cfg->migrateHost && + (STRPREFIX(cfg->migrateHost, "localhost") || + virSocketAddrIsNumericLocalhost(cfg->migrateHost))) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("migration_host must not be the address of" + " the local machine: %s"), + cfg->migrateHost); + return -1; + } + + if (virConfGetValueString(conf, "migration_address", &cfg->migrationAddress) < 0) + return -1; + virStringStripIPv6Brackets(cfg->migrationAddress); + if (cfg->migrationAddress && + (STRPREFIX(cfg->migrationAddress, "localhost") || + virSocketAddrIsNumericLocalhost(cfg->migrationAddress))) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("migration_address must not be the address of" + " the local machine: %s"), + cfg->migrationAddress); + return -1; + } + + return 0; +} +
blank line Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
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 | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8aa5157cd1..b555e33e7b 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,20 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadRPCEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + if (virConfGetValueUInt(conf, "max_queued", &cfg->maxQueuedJobs) < 0) + return -1; + if (virConfGetValueInt(conf, "keepalive_interval", &cfg->keepAliveInterval) < 0) + return -1; + if (virConfGetValueUInt(conf, "keepalive_count", &cfg->keepAliveCount) < 0) + return -1; + + return 0; +} + static int virQEMUDriverConfigLoadNetworkEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf, @@ -946,12 +960,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, VIR_FREE(stdioHandler); } - if (virConfGetValueUInt(conf, "max_queued", &cfg->maxQueuedJobs) < 0) - goto cleanup; - - if (virConfGetValueInt(conf, "keepalive_interval", &cfg->keepAliveInterval) < 0) - goto cleanup; - if (virConfGetValueUInt(conf, "keepalive_count", &cfg->keepAliveCount) < 0) + if (virQEMUDriverConfigLoadRPCEntry(cfg, conf) < 0) goto cleanup; if (virQEMUDriverConfigLoadNetworkEntry(cfg, conf, filename) < 0) -- 2.20.1
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 | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8aa5157cd1..b555e33e7b 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,20 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadRPCEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + if (virConfGetValueUInt(conf, "max_queued", &cfg->maxQueuedJobs) < 0) + return -1; + if (virConfGetValueInt(conf, "keepalive_interval", &cfg->keepAliveInterval) < 0) + return -1; + if (virConfGetValueUInt(conf, "keepalive_count", &cfg->keepAliveCount) < 0) + return -1; + + return 0; +} +
blank line Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
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 | 45 +++++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b555e33e7b..776ba3f3ad 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,31 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadDeviceEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + bool tmp; + int rv; + + if (virConfGetValueBool(conf, "mac_filter", &cfg->macFilter) < 0) + return -1; + + if (virConfGetValueBool(conf, "relaxed_acs_check", &cfg->relaxedACS) < 0) + return -1; + if (virConfGetValueString(conf, "lock_manager", &cfg->lockManagerName) < 0) + return -1; + if ((rv = virConfGetValueBool(conf, "allow_disk_format_probing", &tmp)) < 0) + return -1; + if (rv == 1 && tmp) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("allow_disk_format_probing is no longer supported")); + return -1; + } + + return 0; +} + static int virQEMUDriverConfigLoadRPCEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) @@ -701,7 +726,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, char *stdioHandler = NULL; char **hugetlbfs = NULL; char *corestr = NULL; - bool tmp; /* Just check the file is readable before opening it, otherwise * libvirt emits an error. @@ -900,21 +924,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueString(conf, "pr_helper", &cfg->prHelperName) < 0) goto cleanup; - if (virConfGetValueBool(conf, "mac_filter", &cfg->macFilter) < 0) - goto cleanup; - - if (virConfGetValueBool(conf, "relaxed_acs_check", &cfg->relaxedACS) < 0) - goto cleanup; if (virConfGetValueBool(conf, "clear_emulator_capabilities", &cfg->clearEmulatorCapabilities) < 0) goto cleanup; - - if ((rv = virConfGetValueBool(conf, "allow_disk_format_probing", &tmp)) < 0) - goto cleanup; - if (rv == 1 && tmp) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("allow_disk_format_probing is no longer supported")); - goto cleanup; - } if (virConfGetValueBool(conf, "set_process_name", &cfg->setProcessName) < 0) goto cleanup; if (virConfGetValueUInt(conf, "max_processes", &cfg->maxProcesses) < 0) @@ -940,9 +951,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueBool(conf, "dump_guest_core", &cfg->dumpGuestCore) < 0) goto cleanup; - if (virConfGetValueString(conf, "lock_manager", &cfg->lockManagerName) < 0) - goto cleanup; - if (virConfGetValueString(conf, "stdio_handler", &stdioHandler) < 0) goto cleanup; if (stdioHandler) { @@ -960,6 +968,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, VIR_FREE(stdioHandler); } + if (virQEMUDriverConfigLoadDeviceEntry(cfg, conf) < 0) + goto cleanup; + if (virQEMUDriverConfigLoadRPCEntry(cfg, conf) < 0) goto cleanup; -- 2.20.1
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 | 45 +++++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b555e33e7b..776ba3f3ad 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,31 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadDeviceEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + bool tmp; + int rv; + + if (virConfGetValueBool(conf, "mac_filter", &cfg->macFilter) < 0) + return -1; + + if (virConfGetValueBool(conf, "relaxed_acs_check", &cfg->relaxedACS) < 0) + return -1; + if (virConfGetValueString(conf, "lock_manager", &cfg->lockManagerName) < 0) + return -1; + if ((rv = virConfGetValueBool(conf, "allow_disk_format_probing", &tmp)) < 0) + return -1; + if (rv == 1 && tmp) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("allow_disk_format_probing is no longer supported")); + return -1; + } + + return 0; +} +
blank line Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
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 | 162 ++++++++++++++++++++++++------------------- 1 file changed, 90 insertions(+), 72 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 776ba3f3ad..8bc653fa9e 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,95 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadProcessEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + char *stdioHandler = NULL; + char **hugetlbfs = NULL; + char *corestr = NULL; + int ret = -1; + size_t i; + + if (virConfGetValueStringList(conf, "hugetlbfs_mount", true, + &hugetlbfs) < 0) + goto cleanup; + if (hugetlbfs) { + /* There already might be something autodetected. Avoid leaking it. */ + while (cfg->nhugetlbfs) { + cfg->nhugetlbfs--; + VIR_FREE(cfg->hugetlbfs[cfg->nhugetlbfs].mnt_dir); + } + VIR_FREE(cfg->hugetlbfs); + + cfg->nhugetlbfs = virStringListLength((const char *const *)hugetlbfs); + if (hugetlbfs[0] && + VIR_ALLOC_N(cfg->hugetlbfs, cfg->nhugetlbfs) < 0) + goto cleanup; + + for (i = 0; hugetlbfs[i] != NULL; i++) { + if (virQEMUDriverConfigHugeTLBFSInit(&cfg->hugetlbfs[i], + hugetlbfs[i], i != 0) < 0) + goto cleanup; + } + } + + if (virConfGetValueBool(conf, "clear_emulator_capabilities", &cfg->clearEmulatorCapabilities) < 0) + goto cleanup; + if (virConfGetValueString(conf, "bridge_helper", &cfg->bridgeHelperName) < 0) + goto cleanup; + + if (virConfGetValueString(conf, "pr_helper", &cfg->prHelperName) < 0) + goto cleanup; + + if (virConfGetValueBool(conf, "set_process_name", &cfg->setProcessName) < 0) + goto cleanup; + if (virConfGetValueUInt(conf, "max_processes", &cfg->maxProcesses) < 0) + goto cleanup; + if (virConfGetValueUInt(conf, "max_files", &cfg->maxFiles) < 0) + goto cleanup; + + if (virConfGetValueType(conf, "max_core") == VIR_CONF_STRING) { + if (virConfGetValueString(conf, "max_core", &corestr) < 0) + goto cleanup; + if (STREQ(corestr, "unlimited")) { + cfg->maxCore = ULLONG_MAX; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unknown core size '%s'"), + corestr); + goto cleanup; + } + } else if (virConfGetValueULLong(conf, "max_core", &cfg->maxCore) < 0) { + goto cleanup; + } + + if (virConfGetValueBool(conf, "dump_guest_core", &cfg->dumpGuestCore) < 0) + goto cleanup; + if (virConfGetValueString(conf, "stdio_handler", &stdioHandler) < 0) + goto cleanup; + if (stdioHandler) { + if (STREQ(stdioHandler, "logd")) { + cfg->stdioLogD = true; + } else if (STREQ(stdioHandler, "file")) { + cfg->stdioLogD = false; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unknown stdio handler %s"), + stdioHandler); + VIR_FREE(stdioHandler); + goto cleanup; + } + VIR_FREE(stdioHandler); + } + + ret = 0; + cleanup: + virStringListFree(hugetlbfs); + VIR_FREE(corestr); + return ret; +} + static int virQEMUDriverConfigLoadDeviceEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) @@ -722,8 +811,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, virConfPtr conf = NULL; int ret = -1; int rv; - size_t i; - char *stdioHandler = NULL; char **hugetlbfs = NULL; char *corestr = NULL; @@ -894,79 +981,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (virConfGetValueBool(conf, "auto_start_bypass_cache", &cfg->autoStartBypassCache) < 0) goto cleanup; - - if (virConfGetValueStringList(conf, "hugetlbfs_mount", true, - &hugetlbfs) < 0) - goto cleanup; - if (hugetlbfs) { - /* There already might be something autodetected. Avoid leaking it. */ - while (cfg->nhugetlbfs) { - cfg->nhugetlbfs--; - VIR_FREE(cfg->hugetlbfs[cfg->nhugetlbfs].mnt_dir); - } - VIR_FREE(cfg->hugetlbfs); - - cfg->nhugetlbfs = virStringListLength((const char *const *)hugetlbfs); - if (hugetlbfs[0] && - VIR_ALLOC_N(cfg->hugetlbfs, cfg->nhugetlbfs) < 0) - goto cleanup; - - for (i = 0; hugetlbfs[i] != NULL; i++) { - if (virQEMUDriverConfigHugeTLBFSInit(&cfg->hugetlbfs[i], - hugetlbfs[i], i != 0) < 0) - goto cleanup; - } - } - - if (virConfGetValueString(conf, "bridge_helper", &cfg->bridgeHelperName) < 0) - goto cleanup; - - if (virConfGetValueString(conf, "pr_helper", &cfg->prHelperName) < 0) goto cleanup; - if (virConfGetValueBool(conf, "clear_emulator_capabilities", &cfg->clearEmulatorCapabilities) < 0) - goto cleanup; - if (virConfGetValueBool(conf, "set_process_name", &cfg->setProcessName) < 0) + if (virQEMUDriverConfigLoadProcessEntry(cfg, conf) < 0) goto cleanup; - if (virConfGetValueUInt(conf, "max_processes", &cfg->maxProcesses) < 0) - goto cleanup; - if (virConfGetValueUInt(conf, "max_files", &cfg->maxFiles) < 0) - goto cleanup; - - if (virConfGetValueType(conf, "max_core") == VIR_CONF_STRING) { - if (virConfGetValueString(conf, "max_core", &corestr) < 0) - goto cleanup; - if (STREQ(corestr, "unlimited")) { - cfg->maxCore = ULLONG_MAX; - } else { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown core size '%s'"), - corestr); - goto cleanup; - } - } else if (virConfGetValueULLong(conf, "max_core", &cfg->maxCore) < 0) { - goto cleanup; - } - - if (virConfGetValueBool(conf, "dump_guest_core", &cfg->dumpGuestCore) < 0) - goto cleanup; - - if (virConfGetValueString(conf, "stdio_handler", &stdioHandler) < 0) - goto cleanup; - if (stdioHandler) { - if (STREQ(stdioHandler, "logd")) { - cfg->stdioLogD = true; - } else if (STREQ(stdioHandler, "file")) { - cfg->stdioLogD = false; - } else { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown stdio handler %s"), - stdioHandler); - VIR_FREE(stdioHandler); - goto cleanup; - } - VIR_FREE(stdioHandler); - } if (virQEMUDriverConfigLoadDeviceEntry(cfg, conf) < 0) goto cleanup; -- 2.20.1
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 | 162 ++++++++++++++++++++++++------------------- 1 file changed, 90 insertions(+), 72 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 776ba3f3ad..8bc653fa9e 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,95 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadProcessEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + char *stdioHandler = NULL; + char **hugetlbfs = NULL; + char *corestr = NULL;
VIR_AUTOPTR for each
+ int ret = -1; + size_t i; + + if (virConfGetValueStringList(conf, "hugetlbfs_mount", true, + &hugetlbfs) < 0) + goto cleanup; + if (hugetlbfs) { + /* There already might be something autodetected. Avoid leaking it. */ + while (cfg->nhugetlbfs) { + cfg->nhugetlbfs--; + VIR_FREE(cfg->hugetlbfs[cfg->nhugetlbfs].mnt_dir); + } + VIR_FREE(cfg->hugetlbfs); + + cfg->nhugetlbfs = virStringListLength((const char *const *)hugetlbfs); + if (hugetlbfs[0] && + VIR_ALLOC_N(cfg->hugetlbfs, cfg->nhugetlbfs) < 0) + goto cleanup; + + for (i = 0; hugetlbfs[i] != NULL; i++) { + if (virQEMUDriverConfigHugeTLBFSInit(&cfg->hugetlbfs[i], + hugetlbfs[i], i != 0) < 0) + goto cleanup; + } + } + + if (virConfGetValueBool(conf, "clear_emulator_capabilities", &cfg->clearEmulatorCapabilities) < 0) + goto cleanup; + if (virConfGetValueString(conf, "bridge_helper", &cfg->bridgeHelperName) < 0) + goto cleanup; + + if (virConfGetValueString(conf, "pr_helper", &cfg->prHelperName) < 0) + goto cleanup; + + if (virConfGetValueBool(conf, "set_process_name", &cfg->setProcessName) < 0) + goto cleanup; + if (virConfGetValueUInt(conf, "max_processes", &cfg->maxProcesses) < 0) + goto cleanup; + if (virConfGetValueUInt(conf, "max_files", &cfg->maxFiles) < 0) + goto cleanup; + + if (virConfGetValueType(conf, "max_core") == VIR_CONF_STRING) { + if (virConfGetValueString(conf, "max_core", &corestr) < 0) + goto cleanup; + if (STREQ(corestr, "unlimited")) { + cfg->maxCore = ULLONG_MAX; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unknown core size '%s'"), + corestr); + goto cleanup; + } + } else if (virConfGetValueULLong(conf, "max_core", &cfg->maxCore) < 0) { + goto cleanup; + } + + if (virConfGetValueBool(conf, "dump_guest_core", &cfg->dumpGuestCore) < 0) + goto cleanup; + if (virConfGetValueString(conf, "stdio_handler", &stdioHandler) < 0) + goto cleanup; + if (stdioHandler) { + if (STREQ(stdioHandler, "logd")) { + cfg->stdioLogD = true; + } else if (STREQ(stdioHandler, "file")) { + cfg->stdioLogD = false; + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unknown stdio handler %s"), + stdioHandler); + VIR_FREE(stdioHandler); + goto cleanup; + } + VIR_FREE(stdioHandler); + } + + ret = 0; + cleanup: + virStringListFree(hugetlbfs); + VIR_FREE(corestr); + return ret; +} +
blank line Reviewed-by: John Ferlan <jferlan@redhat.com> John
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 | 162 ++++++++++++++++++++++++------------------- 1 file changed, 90 insertions(+), 72 deletions(-)
I was too quick with the send push... [...]
static int virQEMUDriverConfigLoadDeviceEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) @@ -722,8 +811,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, virConfPtr conf = NULL; int ret = -1; int rv; - size_t i; - char *stdioHandler = NULL; char **hugetlbfs = NULL;
This is now unused and don't forget the StringListFree at the bottom.
char *corestr = NULL;
@@ -894,79 +981,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (virConfGetValueBool(conf, "auto_start_bypass_cache", &cfg->autoStartBypassCache) < 0) goto cleanup; - - if (virConfGetValueStringList(conf, "hugetlbfs_mount", true, - &hugetlbfs) < 0) - goto cleanup; - if (hugetlbfs) { - /* There already might be something autodetected. Avoid leaking it. */ - while (cfg->nhugetlbfs) { - cfg->nhugetlbfs--; - VIR_FREE(cfg->hugetlbfs[cfg->nhugetlbfs].mnt_dir); - } - VIR_FREE(cfg->hugetlbfs); - - cfg->nhugetlbfs = virStringListLength((const char *const *)hugetlbfs); - if (hugetlbfs[0] && - VIR_ALLOC_N(cfg->hugetlbfs, cfg->nhugetlbfs) < 0) - goto cleanup; - - for (i = 0; hugetlbfs[i] != NULL; i++) { - if (virQEMUDriverConfigHugeTLBFSInit(&cfg->hugetlbfs[i], - hugetlbfs[i], i != 0) < 0) - goto cleanup; - } - } - - if (virConfGetValueString(conf, "bridge_helper", &cfg->bridgeHelperName) < 0) - goto cleanup; - - if (virConfGetValueString(conf, "pr_helper", &cfg->prHelperName) < 0) goto cleanup;
This makes a double "goto cleanup;" which you fix in the next patch, but causes this patch to fail to compile... With all that this patch is good. John [...]
On Thu, Jan 17, 2019 at 09:10:11AM -0500, John Ferlan wrote:
On 1/15/19 8:23 AM, Ján Tomko wrote:
@@ -894,79 +981,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (virConfGetValueBool(conf, "auto_start_bypass_cache", &cfg->autoStartBypassCache) < 0) goto cleanup; - - if (virConfGetValueStringList(conf, "hugetlbfs_mount", true, - &hugetlbfs) < 0) - goto cleanup; - if (hugetlbfs) { - /* There already might be something autodetected. Avoid leaking it. */ - while (cfg->nhugetlbfs) { - cfg->nhugetlbfs--; - VIR_FREE(cfg->hugetlbfs[cfg->nhugetlbfs].mnt_dir); - } - VIR_FREE(cfg->hugetlbfs); - - cfg->nhugetlbfs = virStringListLength((const char *const *)hugetlbfs); - if (hugetlbfs[0] && - VIR_ALLOC_N(cfg->hugetlbfs, cfg->nhugetlbfs) < 0) - goto cleanup; - - for (i = 0; hugetlbfs[i] != NULL; i++) { - if (virQEMUDriverConfigHugeTLBFSInit(&cfg->hugetlbfs[i], - hugetlbfs[i], i != 0) < 0) - goto cleanup; - } - } - - if (virConfGetValueString(conf, "bridge_helper", &cfg->bridgeHelperName) < 0) - goto cleanup; - - if (virConfGetValueString(conf, "pr_helper", &cfg->prHelperName) < 0) goto cleanup;
This makes a double "goto cleanup;" which you fix in the next patch, but causes this patch to fail to compile...
Fun, clang does not mind at all. I found another occurrence in a later patch. Jano
With all that this patch is good.
John
[...]
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 | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8bc653fa9e..e03c0b29e3 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,26 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadSaveEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + if (virConfGetValueString(conf, "save_image_format", &cfg->saveImageFormat) < 0) + return -1; + if (virConfGetValueString(conf, "dump_image_format", &cfg->dumpImageFormat) < 0) + return -1; + if (virConfGetValueString(conf, "snapshot_image_format", &cfg->snapshotImageFormat) < 0) + return -1; + if (virConfGetValueString(conf, "auto_dump_path", &cfg->autoDumpPath) < 0) + return -1; + if (virConfGetValueBool(conf, "auto_dump_bypass_cache", &cfg->autoDumpBypassCache) < 0) + return -1; + if (virConfGetValueBool(conf, "auto_start_bypass_cache", &cfg->autoStartBypassCache) < 0) + return -1; + + return 0; +} + static int virQEMUDriverConfigLoadProcessEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) @@ -967,20 +987,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; } - - if (virConfGetValueString(conf, "save_image_format", &cfg->saveImageFormat) < 0) - goto cleanup; - if (virConfGetValueString(conf, "dump_image_format", &cfg->dumpImageFormat) < 0) - goto cleanup; - if (virConfGetValueString(conf, "snapshot_image_format", &cfg->snapshotImageFormat) < 0) - goto cleanup; - - if (virConfGetValueString(conf, "auto_dump_path", &cfg->autoDumpPath) < 0) - goto cleanup; - if (virConfGetValueBool(conf, "auto_dump_bypass_cache", &cfg->autoDumpBypassCache) < 0) - goto cleanup; - if (virConfGetValueBool(conf, "auto_start_bypass_cache", &cfg->autoStartBypassCache) < 0) - goto cleanup; + if (virQEMUDriverConfigLoadSaveEntry(cfg, conf) < 0) goto cleanup; if (virQEMUDriverConfigLoadProcessEntry(cfg, conf) < 0) -- 2.20.1
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 | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8bc653fa9e..e03c0b29e3 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,26 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadSaveEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + if (virConfGetValueString(conf, "save_image_format", &cfg->saveImageFormat) < 0) + return -1; + if (virConfGetValueString(conf, "dump_image_format", &cfg->dumpImageFormat) < 0) + return -1; + if (virConfGetValueString(conf, "snapshot_image_format", &cfg->snapshotImageFormat) < 0) + return -1; + if (virConfGetValueString(conf, "auto_dump_path", &cfg->autoDumpPath) < 0) + return -1; + if (virConfGetValueBool(conf, "auto_dump_bypass_cache", &cfg->autoDumpBypassCache) < 0) + return -1; + if (virConfGetValueBool(conf, "auto_start_bypass_cache", &cfg->autoStartBypassCache) < 0) + return -1; + + return 0; +} +
blank line Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
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 | 131 +++++++++++++++++++++++-------------------- 1 file changed, 71 insertions(+), 60 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e03c0b29e3..317756d2a0 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,76 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadRemoteDisplayEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf, + const char *filename) +{ + if (virConfGetValueUInt(conf, "remote_websocket_port_min", &cfg->webSocketPortMin) < 0) + return -1; + if (cfg->webSocketPortMin < QEMU_WEBSOCKET_PORT_MIN) { + /* if the port is too low, we can't get the display name + * to tell to vnc (usually subtract 5700, e.g. localhost:1 + * for port 5701) */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: remote_websocket_port_min: port must be greater " + "than or equal to %d"), + filename, QEMU_WEBSOCKET_PORT_MIN); + return -1; + } + + if (virConfGetValueUInt(conf, "remote_websocket_port_max", &cfg->webSocketPortMax) < 0) + return -1; + if (cfg->webSocketPortMax > QEMU_WEBSOCKET_PORT_MAX || + cfg->webSocketPortMax < cfg->webSocketPortMin) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: remote_websocket_port_max: port must be between " + "the minimal port and %d"), + filename, QEMU_WEBSOCKET_PORT_MAX); + return -1; + } + + if (cfg->webSocketPortMin > cfg->webSocketPortMax) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: remote_websocket_port_min: min port must not be " + "greater than max port"), filename); + return -1; + } + + if (virConfGetValueUInt(conf, "remote_display_port_min", &cfg->remotePortMin) < 0) + return -1; + if (cfg->remotePortMin < QEMU_REMOTE_PORT_MIN) { + /* if the port is too low, we can't get the display name + * to tell to vnc (usually subtract 5900, e.g. localhost:1 + * for port 5901) */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: remote_display_port_min: port must be greater " + "than or equal to %d"), + filename, QEMU_REMOTE_PORT_MIN); + return -1; + } + + if (virConfGetValueUInt(conf, "remote_display_port_max", &cfg->remotePortMax) < 0) + return -1; + if (cfg->remotePortMax > QEMU_REMOTE_PORT_MAX || + cfg->remotePortMax < cfg->remotePortMin) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: remote_display_port_max: port must be between " + "the minimal port and %d"), + filename, QEMU_REMOTE_PORT_MAX); + return -1; + } + + if (cfg->remotePortMin > cfg->remotePortMax) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: remote_display_port_min: min port must not be " + "greater than max port"), filename); + return -1; + } + + return 0; +} + static int virQEMUDriverConfigLoadSaveEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) @@ -925,67 +995,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, #undef GET_CONFIG_TLS_CERTINFO - if (virConfGetValueUInt(conf, "remote_websocket_port_min", &cfg->webSocketPortMin) < 0) - goto cleanup; - if (cfg->webSocketPortMin < QEMU_WEBSOCKET_PORT_MIN) { - /* if the port is too low, we can't get the display name - * to tell to vnc (usually subtract 5700, e.g. localhost:1 - * for port 5701) */ - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s: remote_websocket_port_min: port must be greater " - "than or equal to %d"), - filename, QEMU_WEBSOCKET_PORT_MIN); - goto cleanup; - } - - if (virConfGetValueUInt(conf, "remote_websocket_port_max", &cfg->webSocketPortMax) < 0) - goto cleanup; - if (cfg->webSocketPortMax > QEMU_WEBSOCKET_PORT_MAX || - cfg->webSocketPortMax < cfg->webSocketPortMin) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s: remote_websocket_port_max: port must be between " - "the minimal port and %d"), - filename, QEMU_WEBSOCKET_PORT_MAX); + if (virQEMUDriverConfigLoadRemoteDisplayEntry(cfg, conf, filename) < 0) goto cleanup; - } - - if (cfg->webSocketPortMin > cfg->webSocketPortMax) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s: remote_websocket_port_min: min port must not be " - "greater than max port"), filename); - goto cleanup; - } - - if (virConfGetValueUInt(conf, "remote_display_port_min", &cfg->remotePortMin) < 0) - goto cleanup; - if (cfg->remotePortMin < QEMU_REMOTE_PORT_MIN) { - /* if the port is too low, we can't get the display name - * to tell to vnc (usually subtract 5900, e.g. localhost:1 - * for port 5901) */ - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s: remote_display_port_min: port must be greater " - "than or equal to %d"), - filename, QEMU_REMOTE_PORT_MIN); - goto cleanup; - } - - if (virConfGetValueUInt(conf, "remote_display_port_max", &cfg->remotePortMax) < 0) - goto cleanup; - if (cfg->remotePortMax > QEMU_REMOTE_PORT_MAX || - cfg->remotePortMax < cfg->remotePortMin) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s: remote_display_port_max: port must be between " - "the minimal port and %d"), - filename, QEMU_REMOTE_PORT_MAX); - goto cleanup; - } - - if (cfg->remotePortMin > cfg->remotePortMax) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s: remote_display_port_min: min port must not be " - "greater than max port"), filename); - goto cleanup; - } if (virQEMUDriverConfigLoadSaveEntry(cfg, conf) < 0) goto cleanup; -- 2.20.1
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 | 131 +++++++++++++++++++++++-------------------- 1 file changed, 71 insertions(+), 60 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index e03c0b29e3..317756d2a0 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,76 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadRemoteDisplayEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf, + const char *filename) +{ + if (virConfGetValueUInt(conf, "remote_websocket_port_min", &cfg->webSocketPortMin) < 0) + return -1; + if (cfg->webSocketPortMin < QEMU_WEBSOCKET_PORT_MIN) { + /* if the port is too low, we can't get the display name + * to tell to vnc (usually subtract 5700, e.g. localhost:1 + * for port 5701) */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: remote_websocket_port_min: port must be greater " + "than or equal to %d"), + filename, QEMU_WEBSOCKET_PORT_MIN); + return -1; + } + + if (virConfGetValueUInt(conf, "remote_websocket_port_max", &cfg->webSocketPortMax) < 0) + return -1; + if (cfg->webSocketPortMax > QEMU_WEBSOCKET_PORT_MAX || + cfg->webSocketPortMax < cfg->webSocketPortMin) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: remote_websocket_port_max: port must be between " + "the minimal port and %d"), + filename, QEMU_WEBSOCKET_PORT_MAX); + return -1; + } + + if (cfg->webSocketPortMin > cfg->webSocketPortMax) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: remote_websocket_port_min: min port must not be " + "greater than max port"), filename); + return -1; + } + + if (virConfGetValueUInt(conf, "remote_display_port_min", &cfg->remotePortMin) < 0) + return -1; + if (cfg->remotePortMin < QEMU_REMOTE_PORT_MIN) { + /* if the port is too low, we can't get the display name + * to tell to vnc (usually subtract 5900, e.g. localhost:1 + * for port 5901) */ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: remote_display_port_min: port must be greater " + "than or equal to %d"), + filename, QEMU_REMOTE_PORT_MIN); + return -1; + } + + if (virConfGetValueUInt(conf, "remote_display_port_max", &cfg->remotePortMax) < 0) + return -1; + if (cfg->remotePortMax > QEMU_REMOTE_PORT_MAX || + cfg->remotePortMax < cfg->remotePortMin) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: remote_display_port_max: port must be between " + "the minimal port and %d"), + filename, QEMU_REMOTE_PORT_MAX); + return -1; + } + + if (cfg->remotePortMin > cfg->remotePortMax) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: remote_display_port_min: min port must not be " + "greater than max port"), filename); + return -1; + } + + return 0; +} +
blank line Reviewed-by: John Ferlan <jferlan@redhat.com> John
Split out parts of the config parsing code to make the parent function easier to read. This is the only patch that mixes various augeas entry groups in one function. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_conf.c | 71 ++++++++++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 317756d2a0..b5024705d8 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,47 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadSpecificTLSEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + int rv; + + if (virConfGetValueBool(conf, "vxhs_tls", &cfg->vxhsTLS) < 0) + return -1; + if (virConfGetValueString(conf, "vxhs_tls_x509_cert_dir", &cfg->vxhsTLSx509certdir) < 0) + return -1; + if (virConfGetValueBool(conf, "nbd_tls", &cfg->nbdTLS) < 0) + return -1; + if (virConfGetValueString(conf, "nbd_tls_x509_cert_dir", &cfg->nbdTLSx509certdir) < 0) + return -1; + +#define GET_CONFIG_TLS_CERTINFO(val) \ + do { \ + if ((rv = virConfGetValueBool(conf, #val "_tls_x509_verify", \ + &cfg->val## TLSx509verify)) < 0) \ + return -1; \ + if (rv == 1) \ + cfg->val## TLSx509verifyPresent = true; \ + if (virConfGetValueString(conf, #val "_tls_x509_cert_dir", \ + &cfg->val## TLSx509certdir) < 0) \ + return -1; \ + if (virConfGetValueString(conf, \ + #val "_tls_x509_secret_uuid", \ + &cfg->val## TLSx509secretUUID) < 0) \ + return -1; \ + } while (0) + + if (virConfGetValueBool(conf, "chardev_tls", &cfg->chardevTLS) < 0) + return -1; + GET_CONFIG_TLS_CERTINFO(chardev); + + GET_CONFIG_TLS_CERTINFO(migrate); + +#undef GET_CONFIG_TLS_CERTINFO + return 0; +} + static int virQEMUDriverConfigLoadRemoteDisplayEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf, @@ -962,38 +1003,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (virConfGetValueBool(conf, "spice_auto_unix_socket", &cfg->spiceAutoUnixSocket) < 0) goto cleanup; - if (virConfGetValueBool(conf, "vxhs_tls", &cfg->vxhsTLS) < 0) - goto cleanup; - if (virConfGetValueString(conf, "vxhs_tls_x509_cert_dir", &cfg->vxhsTLSx509certdir) < 0) - goto cleanup; - if (virConfGetValueBool(conf, "nbd_tls", &cfg->nbdTLS) < 0) - goto cleanup; - if (virConfGetValueString(conf, "nbd_tls_x509_cert_dir", &cfg->nbdTLSx509certdir) < 0) goto cleanup; -#define GET_CONFIG_TLS_CERTINFO(val) \ - do { \ - if ((rv = virConfGetValueBool(conf, #val "_tls_x509_verify", \ - &cfg->val## TLSx509verify)) < 0) \ - goto cleanup; \ - if (rv == 1) \ - cfg->val## TLSx509verifyPresent = true; \ - if (virConfGetValueString(conf, #val "_tls_x509_cert_dir", \ - &cfg->val## TLSx509certdir) < 0) \ - goto cleanup; \ - if (virConfGetValueString(conf, \ - #val "_tls_x509_secret_uuid", \ - &cfg->val## TLSx509secretUUID) < 0) \ - goto cleanup; \ - } while (0) - - if (virConfGetValueBool(conf, "chardev_tls", &cfg->chardevTLS) < 0) + if (virQEMUDriverConfigLoadSpecificTLSEntry(cfg, conf) < 0) goto cleanup; - GET_CONFIG_TLS_CERTINFO(chardev); - - GET_CONFIG_TLS_CERTINFO(migrate); - -#undef GET_CONFIG_TLS_CERTINFO if (virQEMUDriverConfigLoadRemoteDisplayEntry(cfg, conf, filename) < 0) goto cleanup; -- 2.20.1
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.
This is the only patch that mixes various augeas entry groups in one function.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_conf.c | 71 ++++++++++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 29 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 317756d2a0..b5024705d8 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,47 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadSpecificTLSEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + int rv; + + if (virConfGetValueBool(conf, "vxhs_tls", &cfg->vxhsTLS) < 0) + return -1; + if (virConfGetValueString(conf, "vxhs_tls_x509_cert_dir", &cfg->vxhsTLSx509certdir) < 0) + return -1; + if (virConfGetValueBool(conf, "nbd_tls", &cfg->nbdTLS) < 0) + return -1; + if (virConfGetValueString(conf, "nbd_tls_x509_cert_dir", &cfg->nbdTLSx509certdir) < 0) + return -1; + +#define GET_CONFIG_TLS_CERTINFO(val) \ + do { \ + if ((rv = virConfGetValueBool(conf, #val "_tls_x509_verify", \ + &cfg->val## TLSx509verify)) < 0) \ + return -1; \ + if (rv == 1) \ + cfg->val## TLSx509verifyPresent = true; \ + if (virConfGetValueString(conf, #val "_tls_x509_cert_dir", \ + &cfg->val## TLSx509certdir) < 0) \ + return -1; \ + if (virConfGetValueString(conf, \ + #val "_tls_x509_secret_uuid", \ + &cfg->val## TLSx509secretUUID) < 0) \ + return -1; \ + } while (0) + + if (virConfGetValueBool(conf, "chardev_tls", &cfg->chardevTLS) < 0) + return -1; + GET_CONFIG_TLS_CERTINFO(chardev); + + GET_CONFIG_TLS_CERTINFO(migrate); + +#undef GET_CONFIG_TLS_CERTINFO + return 0; +} +
blank line
static int virQEMUDriverConfigLoadRemoteDisplayEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf, @@ -962,38 +1003,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (virConfGetValueBool(conf, "spice_auto_unix_socket", &cfg->spiceAutoUnixSocket) < 0) goto cleanup; - if (virConfGetValueBool(conf, "vxhs_tls", &cfg->vxhsTLS) < 0) - goto cleanup; - if (virConfGetValueString(conf, "vxhs_tls_x509_cert_dir", &cfg->vxhsTLSx509certdir) < 0) - goto cleanup; - if (virConfGetValueBool(conf, "nbd_tls", &cfg->nbdTLS) < 0) - goto cleanup; - if (virConfGetValueString(conf, "nbd_tls_x509_cert_dir", &cfg->nbdTLSx509certdir) < 0) goto cleanup;
This results in a double goto cleanup; ... Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
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 | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b5024705d8..d3f266e338 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,28 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadSPICEEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + if (virConfGetValueBool(conf, "spice_tls", &cfg->spiceTLS) < 0) + return -1; + if (virConfGetValueString(conf, "spice_tls_x509_cert_dir", &cfg->spiceTLSx509certdir) < 0) + return -1; + if (virConfGetValueBool(conf, "spice_sasl", &cfg->spiceSASL) < 0) + return -1; + if (virConfGetValueString(conf, "spice_sasl_dir", &cfg->spiceSASLdir) < 0) + return -1; + if (virConfGetValueString(conf, "spice_listen", &cfg->spiceListen) < 0) + return -1; + if (virConfGetValueString(conf, "spice_password", &cfg->spicePassword) < 0) + return -1; + if (virConfGetValueBool(conf, "spice_auto_unix_socket", &cfg->spiceAutoUnixSocket) < 0) + return -1; + + return 0; +} + static int virQEMUDriverConfigLoadSpecificTLSEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) @@ -988,21 +1010,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueBool(conf, "nographics_allow_host_audio", &cfg->nogfxAllowHostAudio) < 0) goto cleanup; - - if (virConfGetValueBool(conf, "spice_tls", &cfg->spiceTLS) < 0) - goto cleanup; - if (virConfGetValueString(conf, "spice_tls_x509_cert_dir", &cfg->spiceTLSx509certdir) < 0) - goto cleanup; - if (virConfGetValueBool(conf, "spice_sasl", &cfg->spiceSASL) < 0) - goto cleanup; - if (virConfGetValueString(conf, "spice_sasl_dir", &cfg->spiceSASLdir) < 0) - goto cleanup; - if (virConfGetValueString(conf, "spice_listen", &cfg->spiceListen) < 0) - goto cleanup; - if (virConfGetValueString(conf, "spice_password", &cfg->spicePassword) < 0) - goto cleanup; - if (virConfGetValueBool(conf, "spice_auto_unix_socket", &cfg->spiceAutoUnixSocket) < 0) - goto cleanup; + if (virQEMUDriverConfigLoadSPICEEntry(cfg, conf) < 0) goto cleanup; if (virQEMUDriverConfigLoadSpecificTLSEntry(cfg, conf) < 0) -- 2.20.1
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 | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b5024705d8..d3f266e338 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,28 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadSPICEEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + if (virConfGetValueBool(conf, "spice_tls", &cfg->spiceTLS) < 0) + return -1; + if (virConfGetValueString(conf, "spice_tls_x509_cert_dir", &cfg->spiceTLSx509certdir) < 0) + return -1; + if (virConfGetValueBool(conf, "spice_sasl", &cfg->spiceSASL) < 0) + return -1; + if (virConfGetValueString(conf, "spice_sasl_dir", &cfg->spiceSASLdir) < 0) + return -1; + if (virConfGetValueString(conf, "spice_listen", &cfg->spiceListen) < 0) + return -1; + if (virConfGetValueString(conf, "spice_password", &cfg->spicePassword) < 0) + return -1; + if (virConfGetValueBool(conf, "spice_auto_unix_socket", &cfg->spiceAutoUnixSocket) < 0) + return -1; + + return 0; +} +
blank line Reviewed-by: John Ferlan <jferlan@redhat.com> John
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 | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index d3f266e338..62c49c600c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,16 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadNographicsEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + if (virConfGetValueBool(conf, "nographics_allow_host_audio", &cfg->nogfxAllowHostAudio) < 0) + return -1; + + return 0; +} + static int virQEMUDriverConfigLoadSPICEEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) @@ -1007,7 +1017,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (virConfGetValueBool(conf, "vnc_allow_host_audio", &cfg->vncAllowHostAudio) < 0) goto cleanup; - if (virConfGetValueBool(conf, "nographics_allow_host_audio", &cfg->nogfxAllowHostAudio) < 0) + + if (virQEMUDriverConfigLoadNographicsEntry(cfg, conf) < 0) goto cleanup; if (virQEMUDriverConfigLoadSPICEEntry(cfg, conf) < 0) -- 2.20.1
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 | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index d3f266e338..62c49c600c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,16 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadNographicsEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + if (virConfGetValueBool(conf, "nographics_allow_host_audio", &cfg->nogfxAllowHostAudio) < 0) + return -1; + + return 0;
return virConfGetValueBool(...);
+} +
blank line. Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
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 | 50 +++++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 62c49c600c..7eecb3ef91 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,36 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadVNCEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + int rv; + + if (virConfGetValueBool(conf, "vnc_auto_unix_socket", &cfg->vncAutoUnixSocket) < 0) + return -1; + if (virConfGetValueBool(conf, "vnc_tls", &cfg->vncTLS) < 0) + return -1; + if ((rv = virConfGetValueBool(conf, "vnc_tls_x509_verify", &cfg->vncTLSx509verify)) < 0) + return -1; + if (rv == 1) + cfg->vncTLSx509verifyPresent = true; + if (virConfGetValueString(conf, "vnc_tls_x509_cert_dir", &cfg->vncTLSx509certdir) < 0) + return -1; + if (virConfGetValueString(conf, "vnc_listen", &cfg->vncListen) < 0) + return -1; + if (virConfGetValueString(conf, "vnc_password", &cfg->vncPassword) < 0) + return -1; + if (virConfGetValueBool(conf, "vnc_sasl", &cfg->vncSASL) < 0) + return -1; + if (virConfGetValueString(conf, "vnc_sasl_dir", &cfg->vncSASLdir) < 0) + return -1; + if (virConfGetValueBool(conf, "vnc_allow_host_audio", &cfg->vncAllowHostAudio) < 0) + return -1; + + return 0; +} + static int virQEMUDriverConfigLoadNographicsEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) @@ -997,25 +1027,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, &cfg->defaultTLSx509secretUUID) < 0) goto cleanup; - if (virConfGetValueBool(conf, "vnc_auto_unix_socket", &cfg->vncAutoUnixSocket) < 0) - goto cleanup; - if (virConfGetValueBool(conf, "vnc_tls", &cfg->vncTLS) < 0) - goto cleanup; - if ((rv = virConfGetValueBool(conf, "vnc_tls_x509_verify", &cfg->vncTLSx509verify)) < 0) - goto cleanup; - if (rv == 1) - cfg->vncTLSx509verifyPresent = true; - if (virConfGetValueString(conf, "vnc_tls_x509_cert_dir", &cfg->vncTLSx509certdir) < 0) - goto cleanup; - if (virConfGetValueString(conf, "vnc_listen", &cfg->vncListen) < 0) - goto cleanup; - if (virConfGetValueString(conf, "vnc_password", &cfg->vncPassword) < 0) - goto cleanup; - if (virConfGetValueBool(conf, "vnc_sasl", &cfg->vncSASL) < 0) - goto cleanup; - if (virConfGetValueString(conf, "vnc_sasl_dir", &cfg->vncSASLdir) < 0) - goto cleanup; - if (virConfGetValueBool(conf, "vnc_allow_host_audio", &cfg->vncAllowHostAudio) < 0) + if (virQEMUDriverConfigLoadVNCEntry(cfg, conf) < 0) goto cleanup; if (virQEMUDriverConfigLoadNographicsEntry(cfg, conf) < 0) -- 2.20.1
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 | 50 +++++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 19 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 62c49c600c..7eecb3ef91 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,36 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadVNCEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + int rv; + + if (virConfGetValueBool(conf, "vnc_auto_unix_socket", &cfg->vncAutoUnixSocket) < 0) + return -1; + if (virConfGetValueBool(conf, "vnc_tls", &cfg->vncTLS) < 0) + return -1; + if ((rv = virConfGetValueBool(conf, "vnc_tls_x509_verify", &cfg->vncTLSx509verify)) < 0) + return -1; + if (rv == 1) + cfg->vncTLSx509verifyPresent = true; + if (virConfGetValueString(conf, "vnc_tls_x509_cert_dir", &cfg->vncTLSx509certdir) < 0) + return -1; + if (virConfGetValueString(conf, "vnc_listen", &cfg->vncListen) < 0) + return -1; + if (virConfGetValueString(conf, "vnc_password", &cfg->vncPassword) < 0) + return -1; + if (virConfGetValueBool(conf, "vnc_sasl", &cfg->vncSASL) < 0) + return -1; + if (virConfGetValueString(conf, "vnc_sasl_dir", &cfg->vncSASLdir) < 0) + return -1; + if (virConfGetValueBool(conf, "vnc_allow_host_audio", &cfg->vncAllowHostAudio) < 0) + return -1; + + return 0; +} +
blank line Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
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 | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 7eecb3ef91..0877a5b7c1 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,24 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigLoadDefaultTLSEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + int rv; + + if ((rv = virConfGetValueString(conf, "default_tls_x509_cert_dir", &cfg->defaultTLSx509certdir)) < 0) + return -1; + cfg->checkdefaultTLSx509certdir = (rv == 1); + if (virConfGetValueBool(conf, "default_tls_x509_verify", &cfg->defaultTLSx509verify) < 0) + return -1; + if (virConfGetValueString(conf, "default_tls_x509_secret_uuid", + &cfg->defaultTLSx509secretUUID) < 0) + return -1; + + return 0; +} + static int virQEMUDriverConfigLoadVNCEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) @@ -485,6 +503,7 @@ virQEMUDriverConfigLoadSPICEEntry(virQEMUDriverConfigPtr cfg, return 0; } + static int virQEMUDriverConfigLoadSpecificTLSEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) @@ -1003,7 +1022,6 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, { virConfPtr conf = NULL; int ret = -1; - int rv; char **hugetlbfs = NULL; char *corestr = NULL; @@ -1018,13 +1036,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (!(conf = virConfReadFile(filename, 0))) goto cleanup; - if ((rv = virConfGetValueString(conf, "default_tls_x509_cert_dir", &cfg->defaultTLSx509certdir)) < 0) - goto cleanup; - cfg->checkdefaultTLSx509certdir = (rv == 1); - if (virConfGetValueBool(conf, "default_tls_x509_verify", &cfg->defaultTLSx509verify) < 0) - goto cleanup; - if (virConfGetValueString(conf, "default_tls_x509_secret_uuid", - &cfg->defaultTLSx509secretUUID) < 0) + if (virQEMUDriverConfigLoadDefaultTLSEntry(cfg, conf) < 0) goto cleanup; if (virQEMUDriverConfigLoadVNCEntry(cfg, conf) < 0) -- 2.20.1
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 | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 7eecb3ef91..0877a5b7c1 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -423,6 +423,24 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, }
+static int +virQEMUDriverConfigLoadDefaultTLSEntry(virQEMUDriverConfigPtr cfg, + virConfPtr conf) +{ + int rv; + + if ((rv = virConfGetValueString(conf, "default_tls_x509_cert_dir", &cfg->defaultTLSx509certdir)) < 0) + return -1; + cfg->checkdefaultTLSx509certdir = (rv == 1); + if (virConfGetValueBool(conf, "default_tls_x509_verify", &cfg->defaultTLSx509verify) < 0) + return -1; + if (virConfGetValueString(conf, "default_tls_x509_secret_uuid", + &cfg->defaultTLSx509secretUUID) < 0) + return -1; + + return 0; +} +
blank line
static int virQEMUDriverConfigLoadVNCEntry(virQEMUDriverConfigPtr cfg, virConfPtr conf) @@ -485,6 +503,7 @@ virQEMUDriverConfigLoadSPICEEntry(virQEMUDriverConfigPtr cfg, return 0; }
+
Ironically unrelated, but fine IDC. Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
Use defaultTLSx509certdirPresent for consistencty. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_conf.c | 4 ++-- src/qemu/qemu_conf.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 0877a5b7c1..0aa428b87f 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -431,7 +431,7 @@ virQEMUDriverConfigLoadDefaultTLSEntry(virQEMUDriverConfigPtr cfg, if ((rv = virConfGetValueString(conf, "default_tls_x509_cert_dir", &cfg->defaultTLSx509certdir)) < 0) return -1; - cfg->checkdefaultTLSx509certdir = (rv == 1); + cfg->defaultTLSx509certdirPresent = (rv == 1); if (virConfGetValueBool(conf, "default_tls_x509_verify", &cfg->defaultTLSx509verify) < 0) return -1; if (virConfGetValueString(conf, "default_tls_x509_secret_uuid", @@ -1107,7 +1107,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, int virQEMUDriverConfigValidate(virQEMUDriverConfigPtr cfg) { - if (cfg->checkdefaultTLSx509certdir) { + if (cfg->defaultTLSx509certdirPresent) { if (!virFileExists(cfg->defaultTLSx509certdir)) { virReportError(VIR_ERR_CONF_SYNTAX, _("default_tls_x509_cert_dir directory '%s' " diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 87e730058b..bce8364c5a 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -115,7 +115,7 @@ struct _virQEMUDriverConfig { char *swtpmStorageDir; char *defaultTLSx509certdir; - bool checkdefaultTLSx509certdir; + bool defaultTLSx509certdirPresent; bool defaultTLSx509verify; char *defaultTLSx509secretUUID; -- 2.20.1
On 1/15/19 8:23 AM, Ján Tomko wrote:
Use defaultTLSx509certdirPresent for consistencty.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_conf.c | 4 ++-- src/qemu/qemu_conf.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 0aa428b87f..18ad99c173 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -915,7 +915,7 @@ virQEMUDriverConfigLoadSecurityEntry(virQEMUDriverConfigPtr cfg, goto cleanup; if (controllers) { - cfg-> cgroupControllers = 0; + cfg->cgroupControllers = 0; for (i = 0; controllers[i] != NULL; i++) { int ctl; if ((ctl = virCgroupControllerTypeFromString(controllers[i])) < 0) { -- 2.20.1
On 1/15/19 8:23 AM, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Trivial Reviewed-by: John Ferlan <jferlan@redhat.com> John
participants (2)
-
John Ferlan -
Ján Tomko