[libvirt] [PATCH 0/2] Clean up qemu driver configuration parser.
Done while hacking on other series (that is not yet ready). Peter Krempa (2): qemu: Refactor config parameter retrieval qemu: Refactor error reporting in qemu driver configuration parser src/qemu/qemu_conf.c | 479 +++++++++++++++------------------------------------ 1 file changed, 142 insertions(+), 337 deletions(-) -- 1.8.0
This patch adds macros to help retrieve configuration values from qemu driver's configuration. Some configuration options are grouped together in the process. --- src/qemu/qemu_conf.c | 303 +++++++++++++-------------------------------------- 1 file changed, 73 insertions(+), 230 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index b7f249d..84859bf 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -153,50 +153,30 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, return -1; \ } - p = virConfGetValue(conf, "vnc_auto_unix_socket"); - CHECK_TYPE("vnc_auto_unix_socket", VIR_CONF_LONG); - if (p) driver->vncAutoUnixSocket = p->l; - - p = virConfGetValue(conf, "vnc_tls"); - CHECK_TYPE("vnc_tls", VIR_CONF_LONG); - if (p) driver->vncTLS = p->l; - - p = virConfGetValue(conf, "vnc_tls_x509_verify"); - CHECK_TYPE("vnc_tls_x509_verify", VIR_CONF_LONG); - if (p) driver->vncTLSx509verify = p->l; - - p = virConfGetValue(conf, "vnc_tls_x509_cert_dir"); - CHECK_TYPE("vnc_tls_x509_cert_dir", VIR_CONF_STRING); - if (p && p->str) { - VIR_FREE(driver->vncTLSx509certdir); - if (!(driver->vncTLSx509certdir = strdup(p->str))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } - } - - p = virConfGetValue(conf, "vnc_listen"); - CHECK_TYPE("vnc_listen", VIR_CONF_STRING); - if (p && p->str) { - VIR_FREE(driver->vncListen); - if (!(driver->vncListen = strdup(p->str))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } - } - - p = virConfGetValue(conf, "vnc_password"); - CHECK_TYPE("vnc_password", VIR_CONF_STRING); - if (p && p->str) { - VIR_FREE(driver->vncPassword); - if (!(driver->vncPassword = strdup(p->str))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } - } +#define GET_VALUE_LONG(NAME, VAR) p = virConfGetValue(conf, NAME); \ + CHECK_TYPE(NAME, VIR_CONF_LONG); \ + if (p) VAR = p->l; + +#define GET_VALUE_STR(NAME, VAR) p = virConfGetValue(conf, NAME); \ + CHECK_TYPE(NAME, VIR_CONF_STRING); \ + if (p && p->str) { \ + VIR_FREE(VAR); \ + if (!(VAR = strdup(p->str))) { \ + virReportOOMError(); \ + virConfFree(conf); \ + return -1; \ + } \ + } + + GET_VALUE_LONG("vnc_auto_unix_socket", driver->vncAutoUnixSocket); + GET_VALUE_LONG("vnc_tls", driver->vncTLS); + GET_VALUE_LONG("vnc_tls_x509_verify", driver->vncTLSx509verify); + GET_VALUE_STR("vnc_tls_x509_cert_dir", driver->vncTLSx509certdir); + GET_VALUE_STR("vnc_listen", driver->vncListen); + GET_VALUE_STR("vnc_password", driver->vncPassword); + GET_VALUE_LONG("vnc_sasl", driver->vncSASL); + GET_VALUE_STR("vnc_sasl_dir", driver->vncSASLdir); + GET_VALUE_LONG("vnc_allow_host_audio", driver->vncAllowHostAudio); p = virConfGetValue(conf, "security_driver"); if (p && p->type == VIR_CONF_LIST) { @@ -240,104 +220,47 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, } } - p = virConfGetValue(conf, "security_default_confined"); - CHECK_TYPE("security_default_confined", VIR_CONF_LONG); - if (p) driver->securityDefaultConfined = p->l; - - p = virConfGetValue(conf, "security_require_confined"); - CHECK_TYPE("security_require_confined", VIR_CONF_LONG); - if (p) driver->securityRequireConfined = p->l; - - - p = virConfGetValue(conf, "vnc_sasl"); - CHECK_TYPE("vnc_sasl", VIR_CONF_LONG); - if (p) driver->vncSASL = p->l; - - p = virConfGetValue(conf, "vnc_sasl_dir"); - CHECK_TYPE("vnc_sasl_dir", VIR_CONF_STRING); - if (p && p->str) { - VIR_FREE(driver->vncSASLdir); - if (!(driver->vncSASLdir = strdup(p->str))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } - } - - p = virConfGetValue(conf, "spice_tls"); - CHECK_TYPE("spice_tls", VIR_CONF_LONG); - if (p) driver->spiceTLS = p->l; + GET_VALUE_LONG("security_default_confined", driver->securityDefaultConfined); + GET_VALUE_LONG("security_require_confined", driver->securityRequireConfined); - p = virConfGetValue(conf, "spice_tls_x509_cert_dir"); - CHECK_TYPE("spice_tls_x509_cert_dir", VIR_CONF_STRING); - if (p && p->str) { - VIR_FREE(driver->spiceTLSx509certdir); - if (!(driver->spiceTLSx509certdir = strdup(p->str))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } - } + GET_VALUE_LONG("spice_tls", driver->spiceTLS); + GET_VALUE_STR("spice_tls_x509_cert_dir", driver->spiceTLSx509certdir); + GET_VALUE_STR("spice_listen", driver->spiceListen); + GET_VALUE_STR("spice_password", driver->spicePassword); - p = virConfGetValue(conf, "spice_listen"); - CHECK_TYPE("spice_listen", VIR_CONF_STRING); - if (p && p->str) { - VIR_FREE(driver->spiceListen); - if (!(driver->spiceListen = strdup(p->str))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } - } - p = virConfGetValue(conf, "spice_password"); - CHECK_TYPE("spice_password", VIR_CONF_STRING); - if (p && p->str) { - VIR_FREE(driver->spicePassword); - if (!(driver->spicePassword = strdup(p->str))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } - } - - p = virConfGetValue(conf, "remote_display_port_min"); - CHECK_TYPE("remote_display_port_min", VIR_CONF_LONG); - if (p) { - if (p->l < 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); - virConfFree(conf); - return -1; - } - driver->remotePortMin = p->l; + GET_VALUE_LONG("remote_display_port_min", driver->remotePortMin); + if (driver->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); + virConfFree(conf); + return -1; } - p = virConfGetValue(conf, "remote_display_port_max"); - CHECK_TYPE("remote_display_port_max", VIR_CONF_LONG); - if (p) { - if (p->l > QEMU_REMOTE_PORT_MAX || - p->l < driver->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); - virConfFree(conf); - return -1; - } - /* increasing the value by 1 makes all the loops going through - the bitmap (i = remotePortMin; i < remotePortMax; i++), work as - expected. */ - driver->remotePortMax = p->l + 1; + GET_VALUE_LONG("remote_display_port_max", driver->remotePortMax); + if (driver->remotePortMax > QEMU_REMOTE_PORT_MAX || + driver->remotePortMax < driver->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); + virConfFree(conf); + return -1; } + /* increasing the value by 1 makes all the loops going through + the bitmap (i = remotePortMin; i < remotePortMax; i++), work as + expected. */ + driver->remotePortMax += 1; if (driver->remotePortMin > driver->remotePortMax) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("%s: remote_display_port_min: min port must not be greater than max port"), - filename); + _("%s: remote_display_port_min: min port must not be " + "greater than max port"), filename); virConfFree(conf); return -1; } @@ -371,11 +294,7 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, } VIR_FREE(group); - - p = virConfGetValue(conf, "dynamic_ownership"); - CHECK_TYPE("dynamic_ownership", VIR_CONF_LONG); - if (p) driver->dynamicOwnership = p->l; - + GET_VALUE_LONG("dynamic_ownership", driver->dynamicOwnership); p = virConfGetValue(conf, "cgroup_controllers"); CHECK_TYPE("cgroup_controllers", VIR_CONF_LIST); @@ -441,57 +360,13 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, driver->cgroupDeviceACL[i] = NULL; } - p = virConfGetValue(conf, "save_image_format"); - CHECK_TYPE("save_image_format", VIR_CONF_STRING); - if (p && p->str) { - VIR_FREE(driver->saveImageFormat); - if (!(driver->saveImageFormat = strdup(p->str))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } - } - - p = virConfGetValue(conf, "dump_image_format"); - CHECK_TYPE("dump_image_format", VIR_CONF_STRING); - if (p && p->str) { - VIR_FREE(driver->dumpImageFormat); - if (!(driver->dumpImageFormat = strdup(p->str))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } - } - - p = virConfGetValue(conf, "auto_dump_path"); - CHECK_TYPE("auto_dump_path", VIR_CONF_STRING); - if (p && p->str) { - VIR_FREE(driver->autoDumpPath); - if (!(driver->autoDumpPath = strdup(p->str))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } - } - - p = virConfGetValue(conf, "auto_dump_bypass_cache"); - CHECK_TYPE("auto_dump_bypass_cache", VIR_CONF_LONG); - if (p) driver->autoDumpBypassCache = true; + GET_VALUE_STR("save_image_format", driver->saveImageFormat); + GET_VALUE_STR("dump_image_format", driver->dumpImageFormat); + GET_VALUE_STR("auto_dump_path", driver->autoDumpPath); + GET_VALUE_LONG("auto_dump_bypass_cache", driver->autoDumpBypassCache); + GET_VALUE_LONG("auto_start_bypass_cache", driver->autoStartBypassCache); - p = virConfGetValue(conf, "auto_start_bypass_cache"); - CHECK_TYPE("auto_start_bypass_cache", VIR_CONF_LONG); - if (p) driver->autoStartBypassCache = true; - - p = virConfGetValue(conf, "hugetlbfs_mount"); - CHECK_TYPE("hugetlbfs_mount", VIR_CONF_STRING); - if (p && p->str) { - VIR_FREE(driver->hugetlbfs_mount); - if (!(driver->hugetlbfs_mount = strdup(p->str))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } - } + GET_VALUE_STR("hugetlbfs_mount", driver->hugetlbfs_mount); p = virConfGetValue(conf, "mac_filter"); CHECK_TYPE("mac_filter", VIR_CONF_LONG); @@ -515,33 +390,12 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, } } - p = virConfGetValue(conf, "relaxed_acs_check"); - CHECK_TYPE("relaxed_acs_check", VIR_CONF_LONG); - if (p) driver->relaxedACS = p->l; - - p = virConfGetValue(conf, "vnc_allow_host_audio"); - CHECK_TYPE("vnc_allow_host_audio", VIR_CONF_LONG); - if (p) driver->vncAllowHostAudio = p->l; - - p = virConfGetValue(conf, "clear_emulator_capabilities"); - CHECK_TYPE("clear_emulator_capabilities", VIR_CONF_LONG); - if (p) driver->clearEmulatorCapabilities = p->l; - - p = virConfGetValue(conf, "allow_disk_format_probing"); - CHECK_TYPE("allow_disk_format_probing", VIR_CONF_LONG); - if (p) driver->allowDiskFormatProbing = p->l; - - p = virConfGetValue(conf, "set_process_name"); - CHECK_TYPE("set_process_name", VIR_CONF_LONG); - if (p) driver->setProcessName = p->l; - - p = virConfGetValue(conf, "max_processes"); - CHECK_TYPE("max_processes", VIR_CONF_LONG); - if (p) driver->maxProcesses = p->l; - - p = virConfGetValue(conf, "max_files"); - CHECK_TYPE("max_files", VIR_CONF_LONG); - if (p) driver->maxFiles = p->l; + GET_VALUE_LONG("relaxed_acs_check", driver->relaxedACS); + GET_VALUE_LONG("clear_emulator_capabilities", driver->clearEmulatorCapabilities); + GET_VALUE_LONG("allow_disk_format_probing", driver->allowDiskFormatProbing); + GET_VALUE_LONG("set_process_name", driver->setProcessName); + GET_VALUE_LONG("max_processes", driver->maxProcesses); + GET_VALUE_LONG("max_files", driver->maxFiles); p = virConfGetValue(conf, "lock_manager"); CHECK_TYPE("lock_manager", VIR_CONF_STRING); @@ -559,21 +413,10 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, VIR_FREE(lockConf); } - p = virConfGetValue(conf, "max_queued"); - CHECK_TYPE("max_queued", VIR_CONF_LONG); - if (p) driver->max_queued = p->l; - - p = virConfGetValue(conf, "keepalive_interval"); - CHECK_TYPE("keepalive_interval", VIR_CONF_LONG); - if (p) driver->keepAliveInterval = p->l; - - p = virConfGetValue(conf, "keepalive_count"); - CHECK_TYPE("keepalive_count", VIR_CONF_LONG); - if (p) driver->keepAliveCount = p->l; - - p = virConfGetValue(conf, "seccomp_sandbox"); - CHECK_TYPE("seccomp_sandbox", VIR_CONF_LONG); - if (p) driver->seccompSandbox = p->l; + GET_VALUE_LONG("max_queued", driver->max_queued); + GET_VALUE_LONG("keepalive_interval", driver->keepAliveInterval); + GET_VALUE_LONG("keepalive_count", driver->keepAliveCount); + GET_VALUE_LONG("seccomp_sandbox", driver->seccompSandbox); virConfFree(conf); return 0; -- 1.8.0
This patch adds macros to help retrieve configuration values from qemu driver's configuration. Some configuration options are grouped together in the process. --- src/qemu/qemu_conf.c | 303 +++++++++++++-------------------------------------- 1 file changed, 73 insertions(+), 230 deletions(-)
Always fun to see refactoring that reduces code size.
+#define GET_VALUE_LONG(NAME, VAR) p = virConfGetValue(conf, NAME); \
This feels like a long line; I'd format it slightly differently: #define GET_VALUE_LONG(NAME, VAR) \ p = virConfGetValue(conf, NAME); \ ...
+ CHECK_TYPE(NAME, VIR_CONF_LONG); \ + if (p) VAR = p->l;
As long as you are refactoring, can you please split this line into two: if (p) \ VAR = p->l per our coding conventions?
+ +#define GET_VALUE_STR(NAME, VAR) p = virConfGetValue(conf, NAME); \
Again, this feels like a lot of indentation; starting the first line of the macro body after a continuation will get rid of a lot of this whitespace.
+ /* increasing the value by 1 makes all the loops going through + the bitmap (i = remotePortMin; i < remotePortMax; i++), work as + expected. */ + driver->remotePortMax += 1;
Why ' += 1' instead of the shorter '++'?
+ GET_VALUE_LONG("max_queued", driver->max_queued); + GET_VALUE_LONG("keepalive_interval", driver->keepAliveInterval); + GET_VALUE_LONG("keepalive_count", driver->keepAliveCount); + GET_VALUE_LONG("seccomp_sandbox", driver->seccompSandbox);
virConfFree(conf); return 0;
Should we add #undef GET_VALUE_LONG #undef GET_VALUE_STRING at the end, since our macros are merely local helpers for this function? ACK with nits addressed.
On 11/29/12 20:04, Eric Blake wrote:
This patch adds macros to help retrieve configuration values from qemu driver's configuration. Some configuration options are grouped together in the process. --- src/qemu/qemu_conf.c | 303 +++++++++++++-------------------------------------- 1 file changed, 73 insertions(+), 230 deletions(-)
Always fun to see refactoring that reduces code size.
Indeed!
+#define GET_VALUE_LONG(NAME, VAR) p = virConfGetValue(conf, NAME); \
This feels like a long line; I'd format it slightly differently:
#define GET_VALUE_LONG(NAME, VAR) \ p = virConfGetValue(conf, NAME); \ ...
+ CHECK_TYPE(NAME, VIR_CONF_LONG); \ + if (p) VAR = p->l;
As long as you are refactoring, can you please split this line into two:
if (p) \ VAR = p->l
per our coding conventions?
Ah yeah ... I was doing the conversions in a kind of braindead way.
+ +#define GET_VALUE_STR(NAME, VAR) p = virConfGetValue(conf, NAME); \
Again, this feels like a lot of indentation; starting the first line of the macro body after a continuation will get rid of a lot of this whitespace.
+ /* increasing the value by 1 makes all the loops going through + the bitmap (i = remotePortMin; i < remotePortMax; i++), work as + expected. */ + driver->remotePortMax += 1;
Why ' += 1' instead of the shorter '++'?
It was pre-existing but I'm touching the line anyways so I'll change that.
+ GET_VALUE_LONG("max_queued", driver->max_queued); + GET_VALUE_LONG("keepalive_interval", driver->keepAliveInterval); + GET_VALUE_LONG("keepalive_count", driver->keepAliveCount); + GET_VALUE_LONG("seccomp_sandbox", driver->seccompSandbox);
virConfFree(conf); return 0;
Should we add
#undef GET_VALUE_LONG #undef GET_VALUE_STRING
at the end, since our macros are merely local helpers for this function?
Definitely.
ACK with nits addressed.
Pushed, thanks.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
This patch adds two labels and gets rid of a ton of duplicated code. This patch also fixes some error message and swtiches most of them to proper error reporting functions. --- src/qemu/qemu_conf.c | 194 +++++++++++++++++++++------------------------------ 1 file changed, 78 insertions(+), 116 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 84859bf..a086397 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -74,10 +74,11 @@ void qemuDriverUnlock(virQEMUDriverPtr driver) int qemuLoadDriverConfig(virQEMUDriverPtr driver, const char *filename) { - virConfPtr conf; + virConfPtr conf = NULL; virConfValuePtr p; - char *user; - char *group; + char *user = NULL; + char *group = NULL; + int ret = -1; int i; /* Setup critical defaults */ @@ -86,28 +87,21 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, driver->dynamicOwnership = 1; driver->clearEmulatorCapabilities = 1; - if (!(driver->vncListen = strdup("127.0.0.1"))) { - virReportOOMError(); - return -1; - } + if (!(driver->vncListen = strdup("127.0.0.1"))) + goto no_memory; driver->remotePortMin = QEMU_REMOTE_PORT_MIN; driver->remotePortMax = QEMU_REMOTE_PORT_MAX; - if (!(driver->vncTLSx509certdir = strdup(SYSCONFDIR "/pki/libvirt-vnc"))) { - virReportOOMError(); - return -1; - } + if (!(driver->vncTLSx509certdir = strdup(SYSCONFDIR "/pki/libvirt-vnc"))) + goto no_memory; + + if (!(driver->spiceListen = strdup("127.0.0.1"))) + goto no_memory; - if (!(driver->spiceListen = strdup("127.0.0.1"))) { - virReportOOMError(); - return -1; - } if (!(driver->spiceTLSx509certdir - = strdup(SYSCONFDIR "/pki/libvirt-spice"))) { - virReportOOMError(); - return -1; - } + = strdup(SYSCONFDIR "/pki/libvirt-spice"))) + goto no_memory; #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R /* For privileged driver, try and find hugepage mount automatically. @@ -118,14 +112,13 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, if (errno != ENOENT) { virReportSystemError(errno, "%s", _("unable to find hugetlbfs mountpoint")); - return -1; + goto cleanup; } } #endif - if (!(driver->lockManager = - virLockManagerPluginNew("nop", NULL, 0))) - return -1; + if (!(driver->lockManager = virLockManagerPluginNew("nop", NULL, 0))) + goto cleanup; driver->keepAliveInterval = 5; driver->keepAliveCount = 5; @@ -136,21 +129,18 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, */ if (access(filename, R_OK) == -1) { VIR_INFO("Could not read qemu config file %s", filename); - return 0; - } - - conf = virConfReadFile(filename, 0); - if (!conf) { - return -1; + ret = 0; + goto cleanup; } + if (!(conf = virConfReadFile(filename, 0))) + goto cleanup; #define CHECK_TYPE(name,typ) if (p && p->type != (typ)) { \ virReportError(VIR_ERR_INTERNAL_ERROR, \ "%s: %s: expected type " #typ, \ filename, (name)); \ - virConfFree(conf); \ - return -1; \ + goto cleanup; \ } #define GET_VALUE_LONG(NAME, VAR) p = virConfGetValue(conf, NAME); \ @@ -161,11 +151,8 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, CHECK_TYPE(NAME, VIR_CONF_STRING); \ if (p && p->str) { \ VIR_FREE(VAR); \ - if (!(VAR = strdup(p->str))) { \ - virReportOOMError(); \ - virConfFree(conf); \ - return -1; \ - } \ + if (!(VAR = strdup(p->str))) \ + goto no_memory; \ } GET_VALUE_LONG("vnc_auto_unix_socket", driver->vncAutoUnixSocket); @@ -186,36 +173,27 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, /* Calc length and check items */ for (len = 0, pp = p->list; pp; len++, pp = pp->next) { if (pp->type != VIR_CONF_STRING) { - VIR_ERROR(_("security_driver be a list of strings")); - virConfFree(conf); - return -1; + virReportError(VIR_ERR_CONF_SYNTAX, "%s", + _("security_driver must be a list of strings")); + goto cleanup; } } - if (VIR_ALLOC_N(driver->securityDriverNames, len + 1) < 0) { - virReportOOMError(); - virConfFree(conf); - return -1; - } + if (VIR_ALLOC_N(driver->securityDriverNames, len + 1) < 0) + goto no_memory; for (i = 0, pp = p->list; pp; i++, pp = pp->next) { - driver->securityDriverNames[i] = strdup(pp->str); - if (driver->securityDriverNames == NULL) { - virReportOOMError(); - virConfFree(conf); - return -1; - } + if (!(driver->securityDriverNames[i] = strdup(pp->str))) + goto no_memory; } driver->securityDriverNames[len] = NULL; } else { CHECK_TYPE("security_driver", VIR_CONF_STRING); if (p && p->str) { if (VIR_ALLOC_N(driver->securityDriverNames, 2) < 0 || - !(driver->securityDriverNames[0] = strdup(p->str))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } + !(driver->securityDriverNames[0] = strdup(p->str))) + goto no_memory; + driver->securityDriverNames[1] = NULL; } } @@ -238,8 +216,7 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, _("%s: remote_display_port_min: port must be greater " "than or equal to %d"), filename, QEMU_REMOTE_PORT_MIN); - virConfFree(conf); - return -1; + goto cleanup; } GET_VALUE_LONG("remote_display_port_max", driver->remotePortMax); @@ -249,8 +226,7 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, _("%s: remote_display_port_max: port must be between " "the minimal port and %d"), filename, QEMU_REMOTE_PORT_MAX); - virConfFree(conf); - return -1; + goto cleanup; } /* increasing the value by 1 makes all the loops going through the bitmap (i = remotePortMin; i < remotePortMax; i++), work as @@ -261,38 +237,24 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, virReportError(VIR_ERR_INTERNAL_ERROR, _("%s: remote_display_port_min: min port must not be " "greater than max port"), filename); - virConfFree(conf); - return -1; + goto cleanup; } p = virConfGetValue(conf, "user"); CHECK_TYPE("user", VIR_CONF_STRING); - if (!(user = strdup(p && p->str ? p->str : QEMU_USER))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } - if (virGetUserID(user, &driver->user) < 0) { - VIR_FREE(user); - virConfFree(conf); - return -1; - } - VIR_FREE(user); + if (!(user = strdup(p && p->str ? p->str : QEMU_USER))) + goto no_memory; + if (virGetUserID(user, &driver->user) < 0) + goto cleanup; p = virConfGetValue(conf, "group"); CHECK_TYPE("group", VIR_CONF_STRING); - if (!(group = strdup(p && p->str ? p->str : QEMU_GROUP))) { - virReportOOMError(); - virConfFree(conf); - return -1; - } - if (virGetGroupID(group, &driver->group) < 0) { - VIR_FREE(group); - virConfFree(conf); - return -1; - } - VIR_FREE(group); + if (!(group = strdup(p && p->str ? p->str : QEMU_GROUP))) + goto no_memory; + + if (virGetGroupID(group, &driver->group) < 0) + goto cleanup; GET_VALUE_LONG("dynamic_ownership", driver->dynamicOwnership); @@ -303,15 +265,16 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, for (i = 0, pp = p->list; pp; ++i, pp = pp->next) { int ctl; if (pp->type != VIR_CONF_STRING) { - VIR_ERROR(_("cgroup_controllers must be a list of strings")); - virConfFree(conf); - return -1; + virReportError(VIR_ERR_CONF_SYNTAX, "%s", + _("cgroup_controllers must be a " + "list of strings")); + goto cleanup; } - ctl = virCgroupControllerTypeFromString(pp->str); - if (ctl < 0) { - VIR_ERROR(_("Unknown cgroup controller '%s'"), pp->str); - virConfFree(conf); - return -1; + + if ((ctl = virCgroupControllerTypeFromString(pp->str)) < 0) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("Unknown cgroup controller '%s'"), pp->str); + goto cleanup; } driver->cgroupControllers |= (1 << ctl); } @@ -338,24 +301,18 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, virConfValuePtr pp; for (pp = p->list; pp; pp = pp->next) len++; - if (VIR_ALLOC_N(driver->cgroupDeviceACL, 1+len) < 0) { - virReportOOMError(); - virConfFree(conf); - return -1; - } + if (VIR_ALLOC_N(driver->cgroupDeviceACL, 1+len) < 0) + goto no_memory; + for (i = 0, pp = p->list; pp; ++i, pp = pp->next) { if (pp->type != VIR_CONF_STRING) { - VIR_ERROR(_("cgroup_device_acl must be a list of strings")); - virConfFree(conf); - return -1; + virReportError(VIR_ERR_CONF_SYNTAX, "%s", + _("cgroup_device_acl must be a " + "list of strings")); + goto cleanup; } - driver->cgroupDeviceACL[i] = strdup(pp->str); - if (driver->cgroupDeviceACL[i] == NULL) { - virReportOOMError(); - virConfFree(conf); - return -1; - } - + if (!(driver->cgroupDeviceACL[i] = strdup(pp->str))) + goto no_memory; } driver->cgroupDeviceACL[i] = NULL; } @@ -377,16 +334,14 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, virReportSystemError(errno, _("failed to enable mac filter in '%s'"), __FILE__); - virConfFree(conf); - return -1; + goto cleanup; } if ((errno = networkDisableAllFrames(driver))) { virReportSystemError(errno, _("failed to add rule to drop all frames in '%s'"), __FILE__); - virConfFree(conf); - return -1; + goto cleanup; } } @@ -402,11 +357,9 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, if (p && p->str) { char *lockConf; virLockManagerPluginUnref(driver->lockManager); - if (virAsprintf(&lockConf, "%s/libvirt/qemu-%s.conf", SYSCONFDIR, p->str) < 0) { - virReportOOMError(); - virConfFree(conf); - return -1; - } + if (virAsprintf(&lockConf, "%s/libvirt/qemu-%s.conf", SYSCONFDIR, p->str) < 0) + goto no_memory; + if (!(driver->lockManager = virLockManagerPluginNew(p->str, lockConf, 0))) VIR_ERROR(_("Failed to load lock manager %s"), p->str); @@ -418,8 +371,17 @@ int qemuLoadDriverConfig(virQEMUDriverPtr driver, GET_VALUE_LONG("keepalive_count", driver->keepAliveCount); GET_VALUE_LONG("seccomp_sandbox", driver->seccompSandbox); + ret = 0; + +cleanup: + VIR_FREE(user); + VIR_FREE(group); virConfFree(conf); - return 0; + return ret; + +no_memory: + virReportOOMError(); + goto cleanup; } static void -- 1.8.0
This patch adds two labels and gets rid of a ton of duplicated code. This patch also fixes some error message and swtiches most of them to
s/swtiches/switches/
proper error reporting functions. --- src/qemu/qemu_conf.c | 194 +++++++++++++++++++++------------------------------ 1 file changed, 78 insertions(+), 116 deletions(-)
And even more cleanup - nice. ACK.
On 11/29/12 21:14, Eric Blake wrote:
This patch adds two labels and gets rid of a ton of duplicated code. This patch also fixes some error message and swtiches most of them to
s/swtiches/switches/
proper error reporting functions. --- src/qemu/qemu_conf.c | 194 +++++++++++++++++++++------------------------------ 1 file changed, 78 insertions(+), 116 deletions(-)
And even more cleanup - nice.
ACK.
Pushed; Thanks! Peter
participants (2)
-
Eric Blake -
Peter Krempa