5:39 a.m.
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
5:39 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Refactor config parameter retrieval
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
1:04 p.m.
New subject: [libvirt] [PATCH 1/2] qemu: Refactor config parameter retrieval
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.
3:24 p.m.
New subject: [libvirt] [PATCH 1/2] qemu: Refactor config parameter retrieval
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
5:39 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Refactor error reporting in qemu driver configuration parser
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
2:14 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: Refactor error reporting in qemu driver configuration parser
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.
3:24 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: Refactor error reporting in qemu driver configuration parser
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
4377
days inactive
4377
days old
6 comments
2 participants
participants (2)
-
Eric Blake -
Peter Krempa