9:07 a.m.
The daemon-conf test case is terminally broken by design, despite
numerous patches over the years, due to its need to test functionality
indirectly by running libvirtd. Replace it with a test case which
directly unit tests the code in question.
9:07 a.m.
New subject: [libvirt] [PATCH 1/4] Split libvirtd config file loading out into separate files
From: "Daniel P. Berrange" <berrange(a)redhat.com>
To enable creation of unit tests, split the libvirtd config file
loading code out into separate files.
* daemon/libvirtd.c: Delete config loading code / structs
* daemon/libvirtd-config.c, daemon/libvirtd-config.h: Config
file loading APIs
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
daemon/Makefile.am | 1 +
daemon/libvirtd-config.c | 456 +++++++++++++++++++++++++++++++++++++++++++
daemon/libvirtd-config.h | 91 +++++++++
daemon/libvirtd.c | 479 +---------------------------------------------
4 files changed, 549 insertions(+), 478 deletions(-)
create mode 100644 daemon/libvirtd-config.c
create mode 100644 daemon/libvirtd-config.h
diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index db4abf5..9e4b752 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -21,6 +21,7 @@ DAEMON_GENERATED = \
DAEMON_SOURCES = \
libvirtd.c libvirtd.h \
+ libvirtd-config.c libvirtd-config.h \
remote.c remote.h \
stream.c stream.h \
../src/remote/remote_protocol.c \
diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
new file mode 100644
index 0000000..bfb7084
--- /dev/null
+++ b/daemon/libvirtd-config.c
@@ -0,0 +1,456 @@
+/*
+ * libvirtd.c: daemon start of day, guest process & i/o management
+ *
+ * Copyright (C) 2006-2012 Red Hat, Inc.
+ * Copyright (C) 2006 Daniel P. Berrange
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Author: Daniel P. Berrange <berrange(a)redhat.com>
+ */
+
+#include <config.h>
+
+#include "libvirtd-config.h"
+#include "conf.h"
+#include "memory.h"
+#include "virterror_internal.h"
+#include "logging.h"
+#include "virnetserver.h"
+#include "configmake.h"
+#include "remote_protocol.h"
+#include "remote_driver.h"
+
+#define VIR_FROM_THIS VIR_FROM_CONF
+
+/* Allocate an array of malloc'd strings from the config file, filename
+ * (used only in diagnostics), using handle "conf". Upon error, return -1
+ * and free any allocated memory. Otherwise, save the array in *list_arg
+ * and return 0.
+ */
+static int
+remoteConfigGetStringList(virConfPtr conf, const char *key, char ***list_arg,
+ const char *filename)
+{
+ char **list;
+ virConfValuePtr p = virConfGetValue (conf, key);
+ if (!p)
+ return 0;
+
+ switch (p->type) {
+ case VIR_CONF_STRING:
+ if (VIR_ALLOC_N(list, 2) < 0) {
+ VIR_ERROR(_("failed to allocate memory for %s config list"), key);
+ return -1;
+ }
+ list[0] = strdup (p->str);
+ list[1] = NULL;
+ if (list[0] == NULL) {
+ VIR_ERROR(_("failed to allocate memory for %s config list value"),
+ key);
+ VIR_FREE(list);
+ return -1;
+ }
+ break;
+
+ case VIR_CONF_LIST: {
+ int i, len = 0;
+ virConfValuePtr pp;
+ for (pp = p->list; pp; pp = pp->next)
+ len++;
+ if (VIR_ALLOC_N(list, 1+len) < 0) {
+ VIR_ERROR(_("failed to allocate memory for %s config list"), key);
+ return -1;
+ }
+ for (i = 0, pp = p->list; pp; ++i, pp = pp->next) {
+ if (pp->type != VIR_CONF_STRING) {
+ VIR_ERROR(_("remoteReadConfigFile: %s: %s:"
+ " must be a string or list of strings"),
+ filename, key);
+ VIR_FREE(list);
+ return -1;
+ }
+ list[i] = strdup (pp->str);
+ if (list[i] == NULL) {
+ int j;
+ for (j = 0 ; j < i ; j++)
+ VIR_FREE(list[j]);
+ VIR_FREE(list);
+ VIR_ERROR(_("failed to allocate memory for %s config list
value"),
+ key);
+ return -1;
+ }
+
+ }
+ list[i] = NULL;
+ break;
+ }
+
+ default:
+ VIR_ERROR(_("remoteReadConfigFile: %s: %s:"
+ " must be a string or list of strings"),
+ filename, key);
+ return -1;
+ }
+
+ *list_arg = list;
+ return 0;
+}
+
+/* A helper function used by each of the following macros. */
+static int
+checkType (virConfValuePtr p, const char *filename,
+ const char *key, virConfType required_type)
+{
+ if (p->type != required_type) {
+ VIR_ERROR(_("remoteReadConfigFile: %s: %s: invalid type:"
+ " got %s; expected %s"), filename, key,
+ virConfTypeName (p->type),
+ virConfTypeName (required_type));
+ return -1;
+ }
+ return 0;
+}
+
+/* If there is no config data for the key, #var_name, then do nothing.
+ If there is valid data of type VIR_CONF_STRING, and strdup succeeds,
+ store the result in var_name. Otherwise, (i.e. invalid type, or strdup
+ failure), give a diagnostic and "goto" the cleanup-and-fail label. */
+#define GET_CONF_STR(conf, filename, var_name) \
+ do { \
+ virConfValuePtr p = virConfGetValue (conf, #var_name); \
+ if (p) { \
+ if (checkType (p, filename, #var_name, VIR_CONF_STRING) < 0) \
+ goto error; \
+ VIR_FREE(data->var_name); \
+ if (!(data->var_name = strdup (p->str))) { \
+ virReportOOMError(); \
+ goto error; \
+ } \
+ } \
+ } while (0)
+
+/* Like GET_CONF_STR, but for integral values. */
+#define GET_CONF_INT(conf, filename, var_name) \
+ do { \
+ virConfValuePtr p = virConfGetValue (conf, #var_name); \
+ if (p) { \
+ if (checkType (p, filename, #var_name, VIR_CONF_LONG) < 0) \
+ goto error; \
+ data->var_name = p->l; \
+ } \
+ } while (0)
+
+
+static int remoteConfigGetAuth(virConfPtr conf, const char *key, int *auth, const char
*filename) {
+ virConfValuePtr p;
+
+ p = virConfGetValue (conf, key);
+ if (!p)
+ return 0;
+
+ if (checkType (p, filename, key, VIR_CONF_STRING) < 0)
+ return -1;
+
+ if (!p->str)
+ return 0;
+
+ if (STREQ(p->str, "none")) {
+ *auth = VIR_NET_SERVER_SERVICE_AUTH_NONE;
+#if HAVE_SASL
+ } else if (STREQ(p->str, "sasl")) {
+ *auth = VIR_NET_SERVER_SERVICE_AUTH_SASL;
+#endif
+ } else if (STREQ(p->str, "polkit")) {
+ *auth = VIR_NET_SERVER_SERVICE_AUTH_POLKIT;
+ } else {
+ VIR_ERROR(_("remoteReadConfigFile: %s: %s: unsupported auth %s"),
+ filename, key, p->str);
+ return -1;
+ }
+
+ return 0;
+}
+
+int
+daemonConfigFilePath(bool privileged, char **configfile)
+{
+ if (privileged) {
+ if (!(*configfile = strdup(SYSCONFDIR "/libvirt/libvirtd.conf")))
+ goto no_memory;
+ } else {
+ char *userdir = NULL;
+
+ if (!(userdir = virGetUserDirectory(geteuid())))
+ goto error;
+
+ if (virAsprintf(configfile, "%s/.libvirt/libvirtd.conf", userdir) <
0) {
+ VIR_FREE(userdir);
+ goto no_memory;
+ }
+ VIR_FREE(userdir);
+ }
+
+ return 0;
+
+no_memory:
+ virReportOOMError();
+error:
+ return -1;
+}
+
+struct daemonConfig*
+daemonConfigNew(bool privileged ATTRIBUTE_UNUSED)
+{
+ struct daemonConfig *data;
+ char *localhost;
+ int ret;
+
+ if (VIR_ALLOC(data) < 0) {
+ virReportOOMError();
+ return NULL;
+ }
+
+ data->listen_tls = 1;
+ data->listen_tcp = 0;
+
+ if (!(data->tls_port = strdup(LIBVIRTD_TLS_PORT)))
+ goto no_memory;
+ if (!(data->tcp_port = strdup(LIBVIRTD_TCP_PORT)))
+ goto no_memory;
+
+ /* Only default to PolicyKit if running as root */
+#if HAVE_POLKIT
+ if (privileged) {
+ data->auth_unix_rw = REMOTE_AUTH_POLKIT;
+ data->auth_unix_ro = REMOTE_AUTH_POLKIT;
+ } else {
+#endif
+ data->auth_unix_rw = REMOTE_AUTH_NONE;
+ data->auth_unix_ro = REMOTE_AUTH_NONE;
+#if HAVE_POLKIT
+ }
+#endif
+
+ if (data->auth_unix_rw == REMOTE_AUTH_POLKIT)
+ data->unix_sock_rw_perms = strdup("0777"); /* Allow world */
+ else
+ data->unix_sock_rw_perms = strdup("0700"); /* Allow user only */
+ data->unix_sock_ro_perms = strdup("0777"); /* Always allow world */
+ if (!data->unix_sock_ro_perms ||
+ !data->unix_sock_rw_perms)
+ goto no_memory;
+
+#if HAVE_SASL
+ data->auth_tcp = REMOTE_AUTH_SASL;
+#else
+ data->auth_tcp = REMOTE_AUTH_NONE;
+#endif
+ data->auth_tls = REMOTE_AUTH_NONE;
+
+ data->mdns_adv = 0;
+
+ data->min_workers = 5;
+ data->max_workers = 20;
+ data->max_clients = 20;
+
+ data->prio_workers = 5;
+
+ data->max_requests = 20;
+ data->max_client_requests = 5;
+
+ data->log_buffer_size = 64;
+
+ data->audit_level = 1;
+ data->audit_logging = 0;
+
+ data->keepalive_interval = 5;
+ data->keepalive_count = 5;
+ data->keepalive_required = 0;
+
+ localhost = virGetHostname(NULL);
+ if (localhost == NULL) {
+ /* we couldn't resolve the hostname; assume that we are
+ * running in disconnected operation, and report a less
+ * useful Avahi string
+ */
+ ret = virAsprintf(&data->mdns_name, "Virtualization Host");
+ } else {
+ char *tmp;
+ /* Extract the host part of the potentially FQDN */
+ if ((tmp = strchr(localhost, '.')))
+ *tmp = '\0';
+ ret = virAsprintf(&data->mdns_name, "Virtualization Host %s",
+ localhost);
+ }
+ VIR_FREE(localhost);
+ if (ret < 0)
+ goto no_memory;
+
+ return data;
+
+no_memory:
+ virReportOOMError();
+ daemonConfigFree(data);
+ return NULL;
+}
+
+void
+daemonConfigFree(struct daemonConfig *data)
+{
+ char **tmp;
+
+ if (!data)
+ return;
+
+ VIR_FREE(data->listen_addr);
+ VIR_FREE(data->tls_port);
+ VIR_FREE(data->tcp_port);
+
+ VIR_FREE(data->unix_sock_ro_perms);
+ VIR_FREE(data->unix_sock_rw_perms);
+ VIR_FREE(data->unix_sock_group);
+ VIR_FREE(data->unix_sock_dir);
+ VIR_FREE(data->mdns_name);
+
+ tmp = data->tls_allowed_dn_list;
+ while (tmp && *tmp) {
+ VIR_FREE(*tmp);
+ tmp++;
+ }
+ VIR_FREE(data->tls_allowed_dn_list);
+
+ tmp = data->sasl_allowed_username_list;
+ while (tmp && *tmp) {
+ VIR_FREE(*tmp);
+ tmp++;
+ }
+ VIR_FREE(data->sasl_allowed_username_list);
+
+ VIR_FREE(data->key_file);
+ VIR_FREE(data->ca_file);
+ VIR_FREE(data->cert_file);
+ VIR_FREE(data->crl_file);
+
+ VIR_FREE(data->log_filters);
+ VIR_FREE(data->log_outputs);
+
+ VIR_FREE(data);
+}
+
+
+/* Read the config file if it exists.
+ * Only used in the remote case, hence the name.
+ */
+int
+daemonConfigLoad(struct daemonConfig *data,
+ const char *filename,
+ bool allow_missing)
+{
+ virConfPtr conf;
+
+ if (allow_missing &&
+ access(filename, R_OK) == -1 &&
+ errno == ENOENT)
+ return 0;
+
+ conf = virConfReadFile (filename, 0);
+ if (!conf)
+ return -1;
+
+ GET_CONF_INT (conf, filename, listen_tcp);
+ GET_CONF_INT (conf, filename, listen_tls);
+ GET_CONF_STR (conf, filename, tls_port);
+ GET_CONF_STR (conf, filename, tcp_port);
+ GET_CONF_STR (conf, filename, listen_addr);
+
+ if (remoteConfigGetAuth(conf, "auth_unix_rw", &data->auth_unix_rw,
filename) < 0)
+ goto error;
+#if HAVE_POLKIT
+ /* Change default perms to be wide-open if PolicyKit is enabled.
+ * Admin can always override in config file
+ */
+ if (data->auth_unix_rw == REMOTE_AUTH_POLKIT) {
+ VIR_FREE(data->unix_sock_rw_perms);
+ if (!(data->unix_sock_rw_perms = strdup("0777"))) {
+ virReportOOMError();
+ goto error;
+ }
+ }
+#endif
+ if (remoteConfigGetAuth(conf, "auth_unix_ro", &data->auth_unix_ro,
filename) < 0)
+ goto error;
+ if (remoteConfigGetAuth(conf, "auth_tcp", &data->auth_tcp, filename)
< 0)
+ goto error;
+ if (remoteConfigGetAuth(conf, "auth_tls", &data->auth_tls, filename)
< 0)
+ goto error;
+
+ GET_CONF_STR (conf, filename, unix_sock_group);
+ GET_CONF_STR (conf, filename, unix_sock_ro_perms);
+ GET_CONF_STR (conf, filename, unix_sock_rw_perms);
+
+ GET_CONF_STR (conf, filename, unix_sock_dir);
+
+ GET_CONF_INT (conf, filename, mdns_adv);
+ GET_CONF_STR (conf, filename, mdns_name);
+
+ GET_CONF_INT (conf, filename, tls_no_sanity_certificate);
+ GET_CONF_INT (conf, filename, tls_no_verify_certificate);
+
+ GET_CONF_STR (conf, filename, key_file);
+ GET_CONF_STR (conf, filename, cert_file);
+ GET_CONF_STR (conf, filename, ca_file);
+ GET_CONF_STR (conf, filename, crl_file);
+
+ if (remoteConfigGetStringList(conf, "tls_allowed_dn_list",
+ &data->tls_allowed_dn_list, filename) < 0)
+ goto error;
+
+
+ if (remoteConfigGetStringList(conf, "sasl_allowed_username_list",
+ &data->sasl_allowed_username_list, filename)
< 0)
+ goto error;
+
+
+ GET_CONF_INT (conf, filename, min_workers);
+ GET_CONF_INT (conf, filename, max_workers);
+ GET_CONF_INT (conf, filename, max_clients);
+
+ GET_CONF_INT (conf, filename, prio_workers);
+
+ GET_CONF_INT (conf, filename, max_requests);
+ GET_CONF_INT (conf, filename, max_client_requests);
+
+ GET_CONF_INT (conf, filename, audit_level);
+ GET_CONF_INT (conf, filename, audit_logging);
+
+ GET_CONF_STR (conf, filename, host_uuid);
+
+ GET_CONF_INT (conf, filename, log_level);
+ GET_CONF_STR (conf, filename, log_filters);
+ GET_CONF_STR (conf, filename, log_outputs);
+ GET_CONF_INT (conf, filename, log_buffer_size);
+
+ GET_CONF_INT (conf, filename, keepalive_interval);
+ GET_CONF_INT (conf, filename, keepalive_count);
+ GET_CONF_INT (conf, filename, keepalive_required);
+
+ virConfFree (conf);
+ return 0;
+
+error:
+ virConfFree (conf);
+ return -1;
+}
diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h
new file mode 100644
index 0000000..00e8d1c
--- /dev/null
+++ b/daemon/libvirtd-config.h
@@ -0,0 +1,91 @@
+/*
+ * libvirtd.c: daemon start of day, guest process & i/o management
+ *
+ * Copyright (C) 2006-2012 Red Hat, Inc.
+ * Copyright (C) 2006 Daniel P. Berrange
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Author: Daniel P. Berrange <berrange(a)redhat.com>
+ */
+
+#ifndef __LIBVIRTD_CONFIG_H__
+# define __LIBVIRTD_CONFIG_H__
+
+# include "internal.h"
+
+struct daemonConfig {
+ char *host_uuid;
+
+ int listen_tls;
+ int listen_tcp;
+ char *listen_addr;
+ char *tls_port;
+ char *tcp_port;
+
+ char *unix_sock_ro_perms;
+ char *unix_sock_rw_perms;
+ char *unix_sock_group;
+ char *unix_sock_dir;
+
+ int auth_unix_rw;
+ int auth_unix_ro;
+ int auth_tcp;
+ int auth_tls;
+
+ int mdns_adv;
+ char *mdns_name;
+
+ int tls_no_verify_certificate;
+ int tls_no_sanity_certificate;
+ char **tls_allowed_dn_list;
+ char **sasl_allowed_username_list;
+
+ char *key_file;
+ char *cert_file;
+ char *ca_file;
+ char *crl_file;
+
+ int min_workers;
+ int max_workers;
+ int max_clients;
+
+ int prio_workers;
+
+ int max_requests;
+ int max_client_requests;
+
+ int log_level;
+ char *log_filters;
+ char *log_outputs;
+ int log_buffer_size;
+
+ int audit_level;
+ int audit_logging;
+
+ int keepalive_interval;
+ unsigned int keepalive_count;
+ int keepalive_required;
+};
+
+
+int daemonConfigFilePath(bool privileged, char **configfile);
+struct daemonConfig* daemonConfigNew(bool privileged);
+void daemonConfigFree(struct daemonConfig *data);
+int daemonConfigLoad(struct daemonConfig *data,
+ const char *filename,
+ bool allow_missing);
+
+#endif /* __LIBVIRTD_CONFIG_H__ */
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index af07e84..f148777 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -40,11 +40,11 @@
#define VIR_FROM_THIS VIR_FROM_QEMU
#include "libvirtd.h"
+#include "libvirtd-config.h"
#include "util.h"
#include "uuid.h"
#include "remote_driver.h"
-#include "conf.h"
#include "memory.h"
#include "conf.h"
#include "virnetlink.h"
@@ -99,60 +99,6 @@ virNetSASLContextPtr saslCtxt = NULL;
virNetServerProgramPtr remoteProgram = NULL;
virNetServerProgramPtr qemuProgram = NULL;
-struct daemonConfig {
- char *host_uuid;
-
- int listen_tls;
- int listen_tcp;
- char *listen_addr;
- char *tls_port;
- char *tcp_port;
-
- char *unix_sock_ro_perms;
- char *unix_sock_rw_perms;
- char *unix_sock_group;
- char *unix_sock_dir;
-
- int auth_unix_rw;
- int auth_unix_ro;
- int auth_tcp;
- int auth_tls;
-
- int mdns_adv;
- char *mdns_name;
-
- int tls_no_verify_certificate;
- int tls_no_sanity_certificate;
- char **tls_allowed_dn_list;
- char **sasl_allowed_username_list;
-
- char *key_file;
- char *cert_file;
- char *ca_file;
- char *crl_file;
-
- int min_workers;
- int max_workers;
- int max_clients;
-
- int prio_workers;
-
- int max_requests;
- int max_client_requests;
-
- int log_level;
- char *log_filters;
- char *log_outputs;
- int log_buffer_size;
-
- int audit_level;
- int audit_logging;
-
- int keepalive_interval;
- unsigned int keepalive_count;
- int keepalive_required;
-};
-
enum {
VIR_DAEMON_ERR_NONE = 0,
VIR_DAEMON_ERR_PIDFILE,
@@ -592,155 +538,6 @@ static int daemonShutdownCheck(virNetServerPtr srv
ATTRIBUTE_UNUSED,
}
-/* Allocate an array of malloc'd strings from the config file, filename
- * (used only in diagnostics), using handle "conf". Upon error, return -1
- * and free any allocated memory. Otherwise, save the array in *list_arg
- * and return 0.
- */
-static int
-remoteConfigGetStringList(virConfPtr conf, const char *key, char ***list_arg,
- const char *filename)
-{
- char **list;
- virConfValuePtr p = virConfGetValue (conf, key);
- if (!p)
- return 0;
-
- switch (p->type) {
- case VIR_CONF_STRING:
- if (VIR_ALLOC_N(list, 2) < 0) {
- VIR_ERROR(_("failed to allocate memory for %s config list"), key);
- return -1;
- }
- list[0] = strdup (p->str);
- list[1] = NULL;
- if (list[0] == NULL) {
- VIR_ERROR(_("failed to allocate memory for %s config list value"),
- key);
- VIR_FREE(list);
- return -1;
- }
- break;
-
- case VIR_CONF_LIST: {
- int i, len = 0;
- virConfValuePtr pp;
- for (pp = p->list; pp; pp = pp->next)
- len++;
- if (VIR_ALLOC_N(list, 1+len) < 0) {
- VIR_ERROR(_("failed to allocate memory for %s config list"), key);
- return -1;
- }
- for (i = 0, pp = p->list; pp; ++i, pp = pp->next) {
- if (pp->type != VIR_CONF_STRING) {
- VIR_ERROR(_("remoteReadConfigFile: %s: %s:"
- " must be a string or list of strings"),
- filename, key);
- VIR_FREE(list);
- return -1;
- }
- list[i] = strdup (pp->str);
- if (list[i] == NULL) {
- int j;
- for (j = 0 ; j < i ; j++)
- VIR_FREE(list[j]);
- VIR_FREE(list);
- VIR_ERROR(_("failed to allocate memory for %s config list
value"),
- key);
- return -1;
- }
-
- }
- list[i] = NULL;
- break;
- }
-
- default:
- VIR_ERROR(_("remoteReadConfigFile: %s: %s:"
- " must be a string or list of strings"),
- filename, key);
- return -1;
- }
-
- *list_arg = list;
- return 0;
-}
-
-/* A helper function used by each of the following macros. */
-static int
-checkType (virConfValuePtr p, const char *filename,
- const char *key, virConfType required_type)
-{
- if (p->type != required_type) {
- VIR_ERROR(_("remoteReadConfigFile: %s: %s: invalid type:"
- " got %s; expected %s"), filename, key,
- virConfTypeName (p->type),
- virConfTypeName (required_type));
- return -1;
- }
- return 0;
-}
-
-/* If there is no config data for the key, #var_name, then do nothing.
- If there is valid data of type VIR_CONF_STRING, and strdup succeeds,
- store the result in var_name. Otherwise, (i.e. invalid type, or strdup
- failure), give a diagnostic and "goto" the cleanup-and-fail label. */
-#define GET_CONF_STR(conf, filename, var_name) \
- do { \
- virConfValuePtr p = virConfGetValue (conf, #var_name); \
- if (p) { \
- if (checkType (p, filename, #var_name, VIR_CONF_STRING) < 0) \
- goto error; \
- VIR_FREE(data->var_name); \
- if (!(data->var_name = strdup (p->str))) { \
- virReportOOMError(); \
- goto error; \
- } \
- } \
- } while (0)
-
-/* Like GET_CONF_STR, but for integral values. */
-#define GET_CONF_INT(conf, filename, var_name) \
- do { \
- virConfValuePtr p = virConfGetValue (conf, #var_name); \
- if (p) { \
- if (checkType (p, filename, #var_name, VIR_CONF_LONG) < 0) \
- goto error; \
- data->var_name = p->l; \
- } \
- } while (0)
-
-
-static int remoteConfigGetAuth(virConfPtr conf, const char *key, int *auth, const char
*filename) {
- virConfValuePtr p;
-
- p = virConfGetValue (conf, key);
- if (!p)
- return 0;
-
- if (checkType (p, filename, key, VIR_CONF_STRING) < 0)
- return -1;
-
- if (!p->str)
- return 0;
-
- if (STREQ(p->str, "none")) {
- *auth = VIR_NET_SERVER_SERVICE_AUTH_NONE;
-#if HAVE_SASL
- } else if (STREQ(p->str, "sasl")) {
- *auth = VIR_NET_SERVER_SERVICE_AUTH_SASL;
-#endif
- } else if (STREQ(p->str, "polkit")) {
- *auth = VIR_NET_SERVER_SERVICE_AUTH_POLKIT;
- } else {
- VIR_ERROR(_("remoteReadConfigFile: %s: %s: unsupported auth %s"),
- filename, key, p->str);
- return -1;
- }
-
- return 0;
-}
-
/*
* Set up the logging environment
* By default if daemonized all errors go to the logfile libvirtd.log,
@@ -830,280 +627,6 @@ error:
}
-static int
-daemonConfigFilePath(bool privileged, char **configfile)
-{
- if (privileged) {
- if (!(*configfile = strdup(SYSCONFDIR "/libvirt/libvirtd.conf")))
- goto no_memory;
- } else {
- char *userdir = NULL;
-
- if (!(userdir = virGetUserDirectory(geteuid())))
- goto error;
-
- if (virAsprintf(configfile, "%s/.libvirt/libvirtd.conf", userdir) <
0) {
- VIR_FREE(userdir);
- goto no_memory;
- }
- VIR_FREE(userdir);
- }
-
- return 0;
-
-no_memory:
- virReportOOMError();
-error:
- return -1;
-}
-
-static void
-daemonConfigFree(struct daemonConfig *data);
-
-static struct daemonConfig*
-daemonConfigNew(bool privileged ATTRIBUTE_UNUSED)
-{
- struct daemonConfig *data;
- char *localhost;
- int ret;
-
- if (VIR_ALLOC(data) < 0) {
- virReportOOMError();
- return NULL;
- }
-
- data->listen_tls = 1;
- data->listen_tcp = 0;
-
- if (!(data->tls_port = strdup(LIBVIRTD_TLS_PORT)))
- goto no_memory;
- if (!(data->tcp_port = strdup(LIBVIRTD_TCP_PORT)))
- goto no_memory;
-
- /* Only default to PolicyKit if running as root */
-#if HAVE_POLKIT
- if (privileged) {
- data->auth_unix_rw = REMOTE_AUTH_POLKIT;
- data->auth_unix_ro = REMOTE_AUTH_POLKIT;
- } else {
-#endif
- data->auth_unix_rw = REMOTE_AUTH_NONE;
- data->auth_unix_ro = REMOTE_AUTH_NONE;
-#if HAVE_POLKIT
- }
-#endif
-
- if (data->auth_unix_rw == REMOTE_AUTH_POLKIT)
- data->unix_sock_rw_perms = strdup("0777"); /* Allow world */
- else
- data->unix_sock_rw_perms = strdup("0700"); /* Allow user only */
- data->unix_sock_ro_perms = strdup("0777"); /* Always allow world */
- if (!data->unix_sock_ro_perms ||
- !data->unix_sock_rw_perms)
- goto no_memory;
-
-#if HAVE_SASL
- data->auth_tcp = REMOTE_AUTH_SASL;
-#else
- data->auth_tcp = REMOTE_AUTH_NONE;
-#endif
- data->auth_tls = REMOTE_AUTH_NONE;
-
- data->mdns_adv = 0;
-
- data->min_workers = 5;
- data->max_workers = 20;
- data->max_clients = 20;
-
- data->prio_workers = 5;
-
- data->max_requests = 20;
- data->max_client_requests = 5;
-
- data->log_buffer_size = 64;
-
- data->audit_level = 1;
- data->audit_logging = 0;
-
- data->keepalive_interval = 5;
- data->keepalive_count = 5;
- data->keepalive_required = 0;
-
- localhost = virGetHostname(NULL);
- if (localhost == NULL) {
- /* we couldn't resolve the hostname; assume that we are
- * running in disconnected operation, and report a less
- * useful Avahi string
- */
- ret = virAsprintf(&data->mdns_name, "Virtualization Host");
- } else {
- char *tmp;
- /* Extract the host part of the potentially FQDN */
- if ((tmp = strchr(localhost, '.')))
- *tmp = '\0';
- ret = virAsprintf(&data->mdns_name, "Virtualization Host %s",
- localhost);
- }
- VIR_FREE(localhost);
- if (ret < 0)
- goto no_memory;
-
- return data;
-
-no_memory:
- virReportOOMError();
- daemonConfigFree(data);
- return NULL;
-}
-
-static void
-daemonConfigFree(struct daemonConfig *data)
-{
- char **tmp;
-
- if (!data)
- return;
-
- VIR_FREE(data->listen_addr);
- VIR_FREE(data->tls_port);
- VIR_FREE(data->tcp_port);
-
- VIR_FREE(data->unix_sock_ro_perms);
- VIR_FREE(data->unix_sock_rw_perms);
- VIR_FREE(data->unix_sock_group);
- VIR_FREE(data->unix_sock_dir);
- VIR_FREE(data->mdns_name);
-
- tmp = data->tls_allowed_dn_list;
- while (tmp && *tmp) {
- VIR_FREE(*tmp);
- tmp++;
- }
- VIR_FREE(data->tls_allowed_dn_list);
-
- tmp = data->sasl_allowed_username_list;
- while (tmp && *tmp) {
- VIR_FREE(*tmp);
- tmp++;
- }
- VIR_FREE(data->sasl_allowed_username_list);
-
- VIR_FREE(data->key_file);
- VIR_FREE(data->ca_file);
- VIR_FREE(data->cert_file);
- VIR_FREE(data->crl_file);
-
- VIR_FREE(data->log_filters);
- VIR_FREE(data->log_outputs);
-
- VIR_FREE(data);
-}
-
-
-/* Read the config file if it exists.
- * Only used in the remote case, hence the name.
- */
-static int
-daemonConfigLoad(struct daemonConfig *data,
- const char *filename,
- bool allow_missing)
-{
- virConfPtr conf;
-
- if (allow_missing &&
- access(filename, R_OK) == -1 &&
- errno == ENOENT)
- return 0;
-
- conf = virConfReadFile (filename, 0);
- if (!conf)
- return -1;
-
- GET_CONF_INT (conf, filename, listen_tcp);
- GET_CONF_INT (conf, filename, listen_tls);
- GET_CONF_STR (conf, filename, tls_port);
- GET_CONF_STR (conf, filename, tcp_port);
- GET_CONF_STR (conf, filename, listen_addr);
-
- if (remoteConfigGetAuth(conf, "auth_unix_rw", &data->auth_unix_rw,
filename) < 0)
- goto error;
-#if HAVE_POLKIT
- /* Change default perms to be wide-open if PolicyKit is enabled.
- * Admin can always override in config file
- */
- if (data->auth_unix_rw == REMOTE_AUTH_POLKIT) {
- VIR_FREE(data->unix_sock_rw_perms);
- if (!(data->unix_sock_rw_perms = strdup("0777"))) {
- virReportOOMError();
- goto error;
- }
- }
-#endif
- if (remoteConfigGetAuth(conf, "auth_unix_ro", &data->auth_unix_ro,
filename) < 0)
- goto error;
- if (remoteConfigGetAuth(conf, "auth_tcp", &data->auth_tcp, filename)
< 0)
- goto error;
- if (remoteConfigGetAuth(conf, "auth_tls", &data->auth_tls, filename)
< 0)
- goto error;
-
- GET_CONF_STR (conf, filename, unix_sock_group);
- GET_CONF_STR (conf, filename, unix_sock_ro_perms);
- GET_CONF_STR (conf, filename, unix_sock_rw_perms);
-
- GET_CONF_STR (conf, filename, unix_sock_dir);
-
- GET_CONF_INT (conf, filename, mdns_adv);
- GET_CONF_STR (conf, filename, mdns_name);
-
- GET_CONF_INT (conf, filename, tls_no_sanity_certificate);
- GET_CONF_INT (conf, filename, tls_no_verify_certificate);
-
- GET_CONF_STR (conf, filename, key_file);
- GET_CONF_STR (conf, filename, cert_file);
- GET_CONF_STR (conf, filename, ca_file);
- GET_CONF_STR (conf, filename, crl_file);
-
- if (remoteConfigGetStringList(conf, "tls_allowed_dn_list",
- &data->tls_allowed_dn_list, filename) < 0)
- goto error;
-
-
- if (remoteConfigGetStringList(conf, "sasl_allowed_username_list",
- &data->sasl_allowed_username_list, filename)
< 0)
- goto error;
-
-
- GET_CONF_INT (conf, filename, min_workers);
- GET_CONF_INT (conf, filename, max_workers);
- GET_CONF_INT (conf, filename, max_clients);
-
- GET_CONF_INT (conf, filename, prio_workers);
-
- GET_CONF_INT (conf, filename, max_requests);
- GET_CONF_INT (conf, filename, max_client_requests);
-
- GET_CONF_INT (conf, filename, audit_level);
- GET_CONF_INT (conf, filename, audit_logging);
-
- GET_CONF_STR (conf, filename, host_uuid);
-
- GET_CONF_INT (conf, filename, log_level);
- GET_CONF_STR (conf, filename, log_filters);
- GET_CONF_STR (conf, filename, log_outputs);
- GET_CONF_INT (conf, filename, log_buffer_size);
-
- GET_CONF_INT (conf, filename, keepalive_interval);
- GET_CONF_INT (conf, filename, keepalive_count);
- GET_CONF_INT (conf, filename, keepalive_required);
-
- virConfFree (conf);
- return 0;
-
-error:
- virConfFree (conf);
- return -1;
-}
-
/* Display version information. */
static void
daemonVersion(const char *argv0)
--
1.7.7.6
11:56 a.m.
New subject: [libvirt] [PATCH 1/4] Split libvirtd config file loading out into separate files
On 04/04/2012 08:07 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
To enable creation of unit tests, split the libvirtd config file
loading code out into separate files.
* daemon/libvirtd.c: Delete config loading code / structs
* daemon/libvirtd-config.c, daemon/libvirtd-config.h: Config
file loading APIs
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
---
daemon/Makefile.am | 1 +
daemon/libvirtd-config.c | 456 +++++++++++++++++++++++++++++++++++++++++++
daemon/libvirtd-config.h | 91 +++++++++
daemon/libvirtd.c | 479 +---------------------------------------------
4 files changed, 549 insertions(+), 478 deletions(-)
create mode 100644 daemon/libvirtd-config.c
create mode 100644 daemon/libvirtd-config.h
ACK. Looks straightforward.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:07 a.m.
New subject: [libvirt] [PATCH 2/4] Add API for loading daemon config from in-memory blob
From: "Daniel P. Berrange" <berrange(a)redhat.com>
---
daemon/libvirtd-config.c | 67 +++++++++++++++++++++++++++++++--------------
daemon/libvirtd-config.h | 9 ++++--
daemon/libvirtd.c | 2 +-
3 files changed, 53 insertions(+), 25 deletions(-)
diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
index bfb7084..92ae678 100644
--- a/daemon/libvirtd-config.c
+++ b/daemon/libvirtd-config.c
@@ -350,26 +350,11 @@ daemonConfigFree(struct daemonConfig *data)
VIR_FREE(data);
}
-
-/* Read the config file if it exists.
- * Only used in the remote case, hence the name.
- */
-int
-daemonConfigLoad(struct daemonConfig *data,
- const char *filename,
- bool allow_missing)
+static int
+daemonConfigLoadOptions(struct daemonConfig *data,
+ const char *filename,
+ virConfPtr conf)
{
- virConfPtr conf;
-
- if (allow_missing &&
- access(filename, R_OK) == -1 &&
- errno == ENOENT)
- return 0;
-
- conf = virConfReadFile (filename, 0);
- if (!conf)
- return -1;
-
GET_CONF_INT (conf, filename, listen_tcp);
GET_CONF_INT (conf, filename, listen_tls);
GET_CONF_STR (conf, filename, tls_port);
@@ -447,10 +432,50 @@ daemonConfigLoad(struct daemonConfig *data,
GET_CONF_INT (conf, filename, keepalive_count);
GET_CONF_INT (conf, filename, keepalive_required);
- virConfFree (conf);
return 0;
error:
- virConfFree (conf);
return -1;
}
+
+
+/* Read the config file if it exists.
+ * Only used in the remote case, hence the name.
+ */
+int
+daemonConfigLoadFile(struct daemonConfig *data,
+ const char *filename,
+ bool allow_missing)
+{
+ virConfPtr conf;
+ int ret;
+
+ if (allow_missing &&
+ access(filename, R_OK) == -1 &&
+ errno == ENOENT)
+ return 0;
+
+ conf = virConfReadFile(filename, 0);
+ if (!conf)
+ return -1;
+
+ ret = daemonConfigLoadOptions(data, filename, conf);
+ virConfFree (conf);
+ return ret;
+}
+
+int daemonConfigLoadData(struct daemonConfig *data,
+ const char *filename,
+ const char *filedata)
+{
+ virConfPtr conf;
+ int ret;
+
+ conf = virConfReadMem(filedata, strlen(filedata), 0);
+ if (!conf)
+ return -1;
+
+ ret = daemonConfigLoadOptions(data, filename, conf);
+ virConfFree (conf);
+ return ret;
+}
diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h
index 00e8d1c..082cb9c 100644
--- a/daemon/libvirtd-config.h
+++ b/daemon/libvirtd-config.h
@@ -84,8 +84,11 @@ struct daemonConfig {
int daemonConfigFilePath(bool privileged, char **configfile);
struct daemonConfig* daemonConfigNew(bool privileged);
void daemonConfigFree(struct daemonConfig *data);
-int daemonConfigLoad(struct daemonConfig *data,
- const char *filename,
- bool allow_missing);
+int daemonConfigLoadFile(struct daemonConfig *data,
+ const char *filename,
+ bool allow_missing);
+int daemonConfigLoadData(struct daemonConfig *data,
+ const char *filename,
+ const char *filedata);
#endif /* __LIBVIRTD_CONFIG_H__ */
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index f148777..460a552 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -922,7 +922,7 @@ int main(int argc, char **argv) {
/* Read the config file if it exists*/
if (remote_config_file &&
- daemonConfigLoad(config, remote_config_file, implicit_conf) < 0) {
+ daemonConfigLoadFile(config, remote_config_file, implicit_conf) < 0) {
VIR_ERROR(_("Can't load config file '%s'"),
remote_config_file);
exit(EXIT_FAILURE);
}
--
1.7.7.6
11:58 a.m.
New subject: [libvirt] [PATCH 2/4] Add API for loading daemon config from in-memory blob
On 04/04/2012 08:07 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
---
Sparse on the commit message; even mentioning the name of the new API
will help a later 'git log' search for the introduction of the new name.
daemon/libvirtd-config.c | 67
+++++++++++++++++++++++++++++++--------------
daemon/libvirtd-config.h | 9 ++++--
daemon/libvirtd.c | 2 +-
3 files changed, 53 insertions(+), 25 deletions(-)
-
- conf = virConfReadFile (filename, 0);
Bad style (space in function call) here...
- virConfFree (conf);
return 0;
error:
- virConfFree (conf);
and here...
+
+ conf = virConfReadFile(filename, 0);
but while you fixed it here...
+ if (!conf)
+ return -1;
+
+ ret = daemonConfigLoadOptions(data, filename, conf);
+ virConfFree (conf);
...you missed here.
ACK with that nit fixed.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:07 a.m.
New subject: [libvirt] [PATCH 3/4] Switch libvirtd config loading code to use error APIs
From: "Daniel P. Berrange" <berrange(a)redhat.com>
Using VIR_ERROR means the test suite can't catch error messages
easily. Use the proper error reporting APIs instead
---
daemon/libvirtd-config.c | 56 ++++++++++++++++++++++++++++-----------------
daemon/libvirtd.c | 6 ++++-
2 files changed, 40 insertions(+), 22 deletions(-)
diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
index 92ae678..1f4a6fb 100644
--- a/daemon/libvirtd-config.c
+++ b/daemon/libvirtd-config.c
@@ -28,13 +28,17 @@
#include "memory.h"
#include "virterror_internal.h"
#include "logging.h"
-#include "virnetserver.h"
+#include "rpc/virnetserver.h"
#include "configmake.h"
-#include "remote_protocol.h"
-#include "remote_driver.h"
+#include "remote/remote_protocol.h"
+#include "remote/remote_driver.h"
#define VIR_FROM_THIS VIR_FROM_CONF
+#define virConfError(code, ...) \
+ virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \
+ __FUNCTION__, __LINE__, __VA_ARGS__)
+
/* Allocate an array of malloc'd strings from the config file, filename
* (used only in diagnostics), using handle "conf". Upon error, return -1
* and free any allocated memory. Otherwise, save the array in *list_arg
@@ -52,14 +56,17 @@ remoteConfigGetStringList(virConfPtr conf, const char *key, char
***list_arg,
switch (p->type) {
case VIR_CONF_STRING:
if (VIR_ALLOC_N(list, 2) < 0) {
- VIR_ERROR(_("failed to allocate memory for %s config list"), key);
+ virConfError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("failed to allocate memory for %s config list"),
+ key);
return -1;
}
list[0] = strdup (p->str);
list[1] = NULL;
if (list[0] == NULL) {
- VIR_ERROR(_("failed to allocate memory for %s config list value"),
- key);
+ virConfError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("failed to allocate memory for %s config list
value"),
+ key);
VIR_FREE(list);
return -1;
}
@@ -71,14 +78,17 @@ remoteConfigGetStringList(virConfPtr conf, const char *key, char
***list_arg,
for (pp = p->list; pp; pp = pp->next)
len++;
if (VIR_ALLOC_N(list, 1+len) < 0) {
- VIR_ERROR(_("failed to allocate memory for %s config list"), key);
+ virConfError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("failed to allocate memory for %s config list"),
+ key);
return -1;
}
for (i = 0, pp = p->list; pp; ++i, pp = pp->next) {
if (pp->type != VIR_CONF_STRING) {
- VIR_ERROR(_("remoteReadConfigFile: %s: %s:"
- " must be a string or list of strings"),
- filename, key);
+ virConfError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("remoteReadConfigFile: %s: %s:"
+ " must be a string or list of strings"),
+ filename, key);
VIR_FREE(list);
return -1;
}
@@ -88,8 +98,9 @@ remoteConfigGetStringList(virConfPtr conf, const char *key, char
***list_arg,
for (j = 0 ; j < i ; j++)
VIR_FREE(list[j]);
VIR_FREE(list);
- VIR_ERROR(_("failed to allocate memory for %s config list
value"),
- key);
+ virConfError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("failed to allocate memory for %s config list
value"),
+ key);
return -1;
}
@@ -99,9 +110,10 @@ remoteConfigGetStringList(virConfPtr conf, const char *key, char
***list_arg,
}
default:
- VIR_ERROR(_("remoteReadConfigFile: %s: %s:"
- " must be a string or list of strings"),
- filename, key);
+ virConfError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("remoteReadConfigFile: %s: %s:"
+ " must be a string or list of strings"),
+ filename, key);
return -1;
}
@@ -115,10 +127,11 @@ checkType (virConfValuePtr p, const char *filename,
const char *key, virConfType required_type)
{
if (p->type != required_type) {
- VIR_ERROR(_("remoteReadConfigFile: %s: %s: invalid type:"
- " got %s; expected %s"), filename, key,
- virConfTypeName (p->type),
- virConfTypeName (required_type));
+ virConfError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("remoteReadConfigFile: %s: %s: invalid type:"
+ " got %s; expected %s"), filename, key,
+ virConfTypeName (p->type),
+ virConfTypeName (required_type));
return -1;
}
return 0;
@@ -176,8 +189,9 @@ static int remoteConfigGetAuth(virConfPtr conf, const char *key, int
*auth, cons
} else if (STREQ(p->str, "polkit")) {
*auth = VIR_NET_SERVER_SERVICE_AUTH_POLKIT;
} else {
- VIR_ERROR(_("remoteReadConfigFile: %s: %s: unsupported auth %s"),
- filename, key, p->str);
+ virConfError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("remoteReadConfigFile: %s: %s: unsupported auth %s"),
+ filename, key, p->str);
return -1;
}
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 460a552..f4d5029 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -923,7 +923,11 @@ int main(int argc, char **argv) {
/* Read the config file if it exists*/
if (remote_config_file &&
daemonConfigLoadFile(config, remote_config_file, implicit_conf) < 0) {
- VIR_ERROR(_("Can't load config file '%s'"),
remote_config_file);
+ virErrorPtr err = virGetLastError();
+ if (err && err->message)
+ VIR_ERROR(_("%s"), err->message);
+ else
+ VIR_ERROR(_("Can't load config file: %s"),
remote_config_file);
exit(EXIT_FAILURE);
}
--
1.7.7.6
11:59 a.m.
New subject: [libvirt] [PATCH 3/4] Switch libvirtd config loading code to use error APIs
On 04/04/2012 08:07 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
Using VIR_ERROR means the test suite can't catch error messages
easily. Use the proper error reporting APIs instead
---
daemon/libvirtd-config.c | 56 ++++++++++++++++++++++++++++-----------------
daemon/libvirtd.c | 6 ++++-
2 files changed, 40 insertions(+), 22 deletions(-)
+++ b/daemon/libvirtd.c
@@ -923,7 +923,11 @@ int main(int argc, char **argv) {
/* Read the config file if it exists*/
if (remote_config_file &&
daemonConfigLoadFile(config, remote_config_file, implicit_conf) < 0) {
- VIR_ERROR(_("Can't load config file '%s'"),
remote_config_file);
+ virErrorPtr err = virGetLastError();
+ if (err && err->message)
+ VIR_ERROR(_("%s"), err->message);
Huh? _("%s") is useless (no one needs to translate the printf format
that just outputs a string). This should be
VIR_ERROR("%s", err->message);
ACK with that nit fixed.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:07 a.m.
New subject: [libvirt] [PATCH 4/4] Replace daemon-conf test script with a proper test case
From: "Daniel P. Berrange" <berrange(a)redhat.com>
The daemon-conf test script continues to be very fragile to
changes in libvirt. It currently fails 1 time in 3/4 due
to race conditions in startup/shutdown of the test script.
Replace it with a proper test case tailored to the code
being tested
* tests/Makefile.am: Remove daemon-conf, add libvirtdconftest
* tests/daemon-conf: Delete obsolete test
* tests/libvirtdconftest.c: Test config file handling
---
tests/Makefile.am | 16 +++-
tests/daemon-conf | 115 -----------------------
tests/libvirtdconftest.c | 230 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 243 insertions(+), 118 deletions(-)
delete mode 100755 tests/daemon-conf
create mode 100644 tests/libvirtdconftest.c
diff --git a/tests/Makefile.am b/tests/Makefile.am
index dd8bf4f..57358e9 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -170,7 +170,6 @@ if WITH_LIBVIRTD
test_scripts += \
test_conf.sh \
cpuset \
- daemon-conf \
define-dev-segfault \
int-overflow \
libvirtd-fail \
@@ -185,12 +184,13 @@ test_scripts += \
virsh-schedinfo \
virsh-synopsis
-test_programs += eventtest
+test_programs += \
+ eventtest \
+ libvirtdconftest
else
EXTRA_DIST += \
test_conf.sh \
cpuset \
- daemon-conf \
define-dev-segfault \
int-overflow \
libvirtd-fail \
@@ -445,6 +445,16 @@ commandhelper_SOURCES = \
commandhelper_CFLAGS = -Dabs_builddir="\"`pwd`\"" $(AM_CFLAGS)
commandhelper_LDADD = $(LDADDS)
+if WITH_LIBVIRTD
+libvirtdconftest_SOURCES = \
+ libvirtdconftest.c testutils.h testutils.c \
+ ../daemon/libvirtd-config.c
+libvirtdconftest_CFLAGS = $(AM_CFLAGS)
+libvirtdconftest_LDADD = $(LDADDS)
+else
+EXTRA_DIST += libvirtdconftest.c
+endif
+
virnetmessagetest_SOURCES = \
virnetmessagetest.c testutils.h testutils.c
virnetmessagetest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\""
$(AM_CFLAGS)
diff --git a/tests/daemon-conf b/tests/daemon-conf
deleted file mode 100755
index f2b513d..0000000
--- a/tests/daemon-conf
+++ /dev/null
@@ -1,115 +0,0 @@
-#!/bin/sh
-# Get coverage of libvirtd's config-parsing code.
-
-test -z "$srcdir" && srcdir=$(pwd)
-LC_ALL=C
-. "$srcdir/test-lib.sh"
-
-if test "$verbose" = yes; then
- $abs_top_builddir/daemon/libvirtd --version
-fi
-
-test_intro "$this_test"
-
-test -z "$CONFIG_HEADER" &&
CONFIG_HEADER="$abs_top_builddir/config.h"
-
-grep '^#define WITH_QEMU 1' "$CONFIG_HEADER" > /dev/null ||
- skip_test_ "configured without QEMU support"
-
-conf="$abs_top_srcdir/daemon/libvirtd.conf"
-
-# Ensure that each commented out PARAMETER = VALUE line has the expected form.
-grep -v '\"PARAMETER = VALUE\"' "$conf" | grep '[a-z_]
*= *[^ ]' | grep -vE '^#[a-z_]+ = ' \
- && { echo "$0: found unexpected lines (above) in $conf" 1>&2;
exit 1; }
-
-# Start with the sample libvirtd.conf file, uncommenting all real directives.
-sed -n 's/^#\([^ #]\)/\1/p' "$conf" > tmp.conf
-
-sed -e "s/^\(unix_sock_group =\).*/\1 \"$USER\"/g" tmp.conf > k
-mv k tmp.conf
-
-# Iterate through that list of directives, corrupting one RHS at a
-# time and running libvirtd with the resulting config. Each libvirtd
-# invocation must fail.
-n=$(wc -l < tmp.conf)
-i=0
-fail=0
-while :; do
- i=$(expr $i + 1)
-
- param_name=$(sed -n "$i"'s/ = .*//p' tmp.conf)
- rhs=$(sed -n "$i"'s/.* = \(.*\)/\1/p' tmp.conf)
- f=in$i.conf
- case $rhs in
- # Change an RHS that starts with '"' or '[' to "3".
- [[\"]*) sed "$i"'s/ = [["].*/ = 3/' tmp.conf > $f;;
- # Change an RHS that starts with a digit to the string '"foo"'.
- [0-9]*) sed "$i"'s/ = [0-9].*/ = "foo"/' tmp.conf >
$f;;
- esac
-
- # Run libvirtd, expecting it to fail.
- $abs_top_builddir/daemon/libvirtd --pid-file=pid-file --config=$f 2> err &&
fail=1
-
- case $rhs in
- # '"'*) msg='should be a string';;
- '"'*) msg='invalid type: got long; expected string';;
- [0-9]*) msg='invalid type: got string; expected long';;
- '['*) msg='must be a string or list of strings';;
- *) echo "unexpected RHS: $rhs" 1>&2; fail=1;;
- esac
-
- test $i = $n && break
-
- # Check that the diagnostic we want appears
- grep "$msg" err 1>/dev/null 2>&1
- RET=$?
- test_result $i "corrupted config $param_name" $RET
- test "$RET" = "0" || fail=1
-done
-
-# Run with the unmodified config file.
-sleep_secs=2
-
-# Be careful to specify a non-default socket directory:
-sed 's,^unix_sock_dir.*,unix_sock_dir="'"$(pwd)"'",'
tmp.conf > k || fail=1
-mv k tmp.conf || fail=1
-
-# Also, specify a test-specific log directory:
-sed
's,^log_outputs.*,log_outputs="3:file:'"$(pwd)/log"'",'
tmp.conf > k \
- || fail=1
-mv k tmp.conf || fail=1
-
-# Unix socket max path size is 108 on linux. If the generated sock path
-# exceeds this, the test will fail, so skip it if CWD is too long
-SOCKPATH=`pwd`/libvirt-sock
-if test 108 -lt `echo $SOCKPATH | wc -c`; then
- skip_test_ "CWD too long"
-fi
-
-# Replace the invalid host_uuid with one that is valid and
-# relax audit_level from 2 to 1, otherwise libvirtd will report an error
-# when auditing is disabled on the host or during compilation
-sed 's/^\(host_uuid =.*\)0"$/\11"/; s/^\(audit_level =.*\)2$/\1 1/'
tmp.conf > k
-mv k tmp.conf
-
-$abs_top_builddir/daemon/libvirtd --pid-file=pid-file --config=tmp.conf \
- > log 2>&1 & pid=$!
-sleep $sleep_secs
-kill $pid
-
-RET=0
-# Expect an orderly shut-down and successful exit.
-wait $pid || RET=1
-
-test_result $i "valid config file (sleeping $sleep_secs seconds)" $RET
-test $RET = 0 || fail=1
-
-test_final $i $fail
-
-# "cat log" would print this for non-root:
-# Cannot set group when not running as root
-# Shutting down on signal 15
-# And normally, we'd require that output, but obviously
-# it'd be different for root.
-
-exit $fail
diff --git a/tests/libvirtdconftest.c b/tests/libvirtdconftest.c
new file mode 100644
index 0000000..f97bf80
--- /dev/null
+++ b/tests/libvirtdconftest.c
@@ -0,0 +1,230 @@
+/*
+ * Copyright (C) 2012 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Author: Daniel P. Berrange <berrange(a)redhat.com>
+ */
+
+#include <config.h>
+
+#include <stdlib.h>
+
+#include "testutils.h"
+#include "daemon/libvirtd-config.h"
+#include "util.h"
+#include "c-ctype.h"
+#include "virterror_internal.h"
+#include "logging.h"
+#include "conf.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
+
+struct testCorruptData {
+ size_t *params;
+ const char *filedata;
+ const char *filename;
+ size_t paramnum;
+};
+
+static char *
+munge_param(const char *datain,
+ size_t *params,
+ size_t paramnum,
+ int *type)
+{
+ char *dataout;
+ const char *sol;
+ const char *eol;
+ const char *eq;
+ const char *tmp;
+ size_t dataoutlen;
+ const char *replace = NULL;
+
+ sol = datain + params[paramnum];
+ eq = strchr(sol, '=');
+ eol = strchr(sol, '\n');
+
+ for (tmp = eq + 1; tmp < eol && !replace; tmp++) {
+ if (c_isspace(*tmp))
+ continue;
+ if (c_isdigit(*tmp)) {
+ *type = VIR_CONF_LONG;
+ replace = "\"foo\"";
+ } else if (*tmp == '[') {
+ *type = VIR_CONF_LIST;
+ replace = "666";
+ } else {
+ *type = VIR_CONF_STRING;
+ replace = "666";
+ }
+ }
+
+ dataoutlen = (eq - datain) + 1 +
+ strlen(replace) +
+ strlen(eol) + 1;
+
+ if (VIR_ALLOC_N(dataout, dataoutlen) < 0) {
+ virReportOOMError();
+ return NULL;
+ }
+ memcpy(dataout, datain, (eq - datain) + 1);
+ memcpy(dataout + (eq - datain) + 1,
+ replace, strlen(replace));
+ memcpy(dataout + (eq - datain) + 1 + strlen(replace),
+ eol, strlen(eol) + 1);
+
+ return dataout;
+}
+
+static int
+testCorrupt(const void *opaque)
+{
+ const struct testCorruptData *data = opaque;
+ struct daemonConfig *conf = daemonConfigNew(false);
+ int ret = 0;
+ int type = VIR_CONF_NONE;
+ char *newdata = munge_param(data->filedata,
+ data->params,
+ data->paramnum,
+ &type);
+ virErrorPtr err = NULL;
+
+ if (!newdata)
+ return -1;
+
+ //VIR_DEBUG("New config [%s]", newdata);
+
+ if (daemonConfigLoadData(conf, data->filename, newdata) != -1) {
+ VIR_DEBUG("Did not see a failure");
+ ret = -1;
+ goto cleanup;
+ }
+
+ err = virGetLastError();
+ if (!err || !err->message) {
+ VIR_DEBUG("No error or message %p", err);
+ ret = -1;
+ goto cleanup;
+ }
+
+ switch (type) {
+ case VIR_CONF_LONG:
+ if (!strstr(err->message, "invalid type: got string; expected
long")) {
+ VIR_DEBUG("Wrong error for long: '%s'",
+ err->message);
+ ret = -1;
+ }
+ break;
+ case VIR_CONF_STRING:
+ if (!strstr(err->message, "invalid type: got long; expected
string")) {
+ VIR_DEBUG("Wrong error for string: '%s'",
+ err->message);
+ ret = -1;
+ }
+ break;
+ case VIR_CONF_LIST:
+ if (!strstr(err->message, "must be a string or list of strings")) {
+ VIR_DEBUG("Wrong error for list: '%s'",
+ err->message);
+ ret = -1;
+ }
+ break;
+ }
+
+cleanup:
+ VIR_FREE(newdata);
+ daemonConfigFree(conf);
+ return ret;
+}
+
+static int
+uncomment_all_params(char *data,
+ size_t **ret)
+{
+ size_t count = 0;
+ char *tmp;
+ size_t *params = 0;
+
+ tmp = data;
+ while (tmp && *tmp) {
+ tmp = strchr(tmp, '\n');
+ if (!tmp)
+ break;
+
+ tmp++;
+
+ /* Uncomment any lines starting #some_var */
+ if (*tmp == '#' &&
+ c_isalpha(*(tmp + 1))) {
+ if (VIR_EXPAND_N(params, count, 1) < 0) {
+ VIR_FREE(params);
+ return -1;
+ }
+ *tmp = ' ';
+ params[count-1] = (tmp + 1) - data;
+ }
+ }
+ if (VIR_EXPAND_N(params, count, 1) < 0) {
+ VIR_FREE(params);
+ return -1;
+ }
+ params[count-1] = 0;
+ *ret = params;
+ return count;
+}
+
+static int
+mymain(void)
+{
+ int ret = 0;
+ char *filedata = NULL;
+ char *filename = NULL;
+ size_t i;
+ size_t *params = NULL;
+
+ if (virAsprintf(&filename, "%s/../daemon/libvirtd.conf",
+ abs_srcdir) < 0) {
+ perror("Format filename");
+ return EXIT_FAILURE;
+ }
+
+ if (virFileReadAll(filename, 1024*1024, &filedata) < 0) {
+ virErrorPtr err = virGetLastError();
+ fprintf(stderr, "Cannot load %s for testing: %s", filename,
err->message);
+ ret = -1;
+ goto cleanup;
+ }
+
+ if (uncomment_all_params(filedata, ¶ms) < 0){
+ perror("Find params");
+ ret = -1;
+ goto cleanup;
+ }
+ VIR_DEBUG("Initial config [%s]", filedata);
+ for (i = 0 ; params[i] != 0 ; i++) {
+ const struct testCorruptData data = { params, filedata, filename, i };
+ if (virtTestRun("Test corruption", 1, testCorrupt, &data) < 0)
+ ret = -1;
+ }
+
+cleanup:
+ VIR_FREE(filename);
+ VIR_FREE(filedata);
+ VIR_FREE(params);
+ return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE;
+}
+
+VIRT_TEST_MAIN(mymain)
--
1.7.7.6
12:05 p.m.
New subject: [libvirt] [PATCH 4/4] Replace daemon-conf test script with a proper test case
On 04/04/2012 08:07 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)redhat.com>
The daemon-conf test script continues to be very fragile to
changes in libvirt. It currently fails 1 time in 3/4 due
to race conditions in startup/shutdown of the test script.
Replace it with a proper test case tailored to the code
being tested
* tests/Makefile.am: Remove daemon-conf, add libvirtdconftest
* tests/daemon-conf: Delete obsolete test
* tests/libvirtdconftest.c: Test config file handling
---
tests/Makefile.am | 16 +++-
tests/daemon-conf | 115 -----------------------
tests/libvirtdconftest.c | 230 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 243 insertions(+), 118 deletions(-)
delete mode 100755 tests/daemon-conf
create mode 100644 tests/libvirtdconftest.c
Definitely worth doing!!!
Missing a change to the top-level .gitignore to manage the new test name.
+
+ if (VIR_ALLOC_N(dataout, dataoutlen) < 0) {
+ virReportOOMError();
+ return NULL;
+ }
+ memcpy(dataout, datain, (eq - datain) + 1);
+ memcpy(dataout + (eq - datain) + 1,
+ replace, strlen(replace));
I might have written this as
strcpy(dataout + (eq - datain) + 1, replace)
for one less pass over replace, but it's not worth the micro-optimization.
+
+ //VIR_DEBUG("New config [%s]", newdata);
Did you mean to leave this commented?
ACK with the nits fixed.
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:13 a.m.
New subject: [libvirt] [PATCH 4/4] Replace daemon-conf test script with a proper test case
On Fri, Apr 06, 2012 at 11:05:02AM -0600, Eric Blake wrote:
On 04/04/2012 08:07 AM, Daniel P. Berrange wrote:
> +
> + //VIR_DEBUG("New config [%s]", newdata);
Did you mean to leave this commented?
Yes, it makes the debug output far too verbose.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
4639
days inactive
4645
days old
9 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Eric Blake