[libvirt] [PATCH 0/4] Replace daemon-conf test case

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.

From: "Daniel P. Berrange" <berrange@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@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@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@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

On 04/04/2012 08:07 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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@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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: "Daniel P. Berrange" <berrange@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

On 04/04/2012 08:07 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: "Daniel P. Berrange" <berrange@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

On 04/04/2012 08:07 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: "Daniel P. Berrange" <berrange@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@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

On 04/04/2012 08:07 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

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 :|
participants (2)
-
Daniel P. Berrange
-
Eric Blake