virConnectGetConfig and virConnectGetConfigPath were static libvirt methods, merely because there hasn't been any need for having them internally exported yet. Since libvirt-admin also needs to reference libvirt config file, 'xGetConfig' should be exported. Besides moving, this patch also renames the methods accordingly, as they are libvirt config specific. --- src/libvirt.c | 55 +----------------------------------------------- src/libvirt_private.syms | 1 + src/util/virconf.c | 52 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virconf.h | 2 +- 4 files changed, 55 insertions(+), 55 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 2602dde..5ee41ef 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -908,59 +908,6 @@ virGetVersion(unsigned long *libVer, const char *type ATTRIBUTE_UNUSED, return -1; } - -static char * -virConnectGetConfigFilePath(void) -{ - char *path; - if (geteuid() == 0) { - if (virAsprintf(&path, "%s/libvirt/libvirt.conf", - SYSCONFDIR) < 0) - return NULL; - } else { - char *userdir = virGetUserConfigDirectory(); - if (!userdir) - return NULL; - - if (virAsprintf(&path, "%s/libvirt.conf", - userdir) < 0) { - VIR_FREE(userdir); - return NULL; - } - VIR_FREE(userdir); - } - - return path; -} - - -static int -virConnectGetConfigFile(virConfPtr *conf) -{ - char *filename = NULL; - int ret = -1; - - *conf = NULL; - - if (!(filename = virConnectGetConfigFilePath())) - goto cleanup; - - if (!virFileExists(filename)) { - ret = 0; - goto cleanup; - } - - VIR_DEBUG("Loading config file '%s'", filename); - if (!(*conf = virConfReadFile(filename, 0))) - goto cleanup; - - ret = 0; - - cleanup: - VIR_FREE(filename); - return ret; -} - #define URI_ALIAS_CHARS "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_-" @@ -1078,7 +1025,7 @@ do_open(const char *name, if (ret == NULL) return NULL; - if (virConnectGetConfigFile(&conf) < 0) + if (virGetLibvirtConfigFile(&conf) < 0) goto failed; if (name && name[0] == '\0') diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index be6ee19..b72a0ee 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1332,6 +1332,7 @@ virConfTypeToString; virConfWalk; virConfWriteFile; virConfWriteMem; +virGetLibvirtConfigFile; # util/vircrypto.h diff --git a/src/util/virconf.c b/src/util/virconf.c index 9f2d116..0b3d7da 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -38,6 +38,7 @@ #include "viralloc.h" #include "virfile.h" #include "virstring.h" +#include "configmake.h" #define VIR_FROM_THIS VIR_FROM_CONF @@ -1053,3 +1054,54 @@ virConfWriteMem(char *memory, int *len, virConfPtr conf) *len = use; return use; } + +static char * +virGetLibvirtConfigFilePath(void) +{ + char *path; + if (geteuid() == 0) { + if (virAsprintf(&path, "%s/libvirt/libvirt.conf", + SYSCONFDIR) < 0) + return NULL; + } else { + char *userdir = virGetUserConfigDirectory(); + if (!userdir) + return NULL; + + if (virAsprintf(&path, "%s/libvirt.conf", + userdir) < 0) { + VIR_FREE(userdir); + return NULL; + } + VIR_FREE(userdir); + } + + return path; +} + +int +virGetLibvirtConfigFile(virConfPtr *conf) +{ + char *filename = NULL; + int ret = -1; + + *conf = NULL; + + if (!(filename = virGetLibvirtConfigFilePath())) + goto cleanup; + + if (!virFileExists(filename)) { + ret = 0; + goto cleanup; + } + + VIR_DEBUG("Loading config file '%s'", filename); + if (!(*conf = virConfReadFile(filename, 0))) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(filename); + return ret; +} diff --git a/src/util/virconf.h b/src/util/virconf.h index 8037956..dad7dba 100644 --- a/src/util/virconf.h +++ b/src/util/virconf.h @@ -96,5 +96,5 @@ int virConfWriteFile(const char *filename, int virConfWriteMem(char *memory, int *len, virConfPtr conf); - +int virGetLibvirtConfigFile(virConfPtr *conf); #endif /* __VIR_CONF_H__ */ -- 2.4.3

This patch introduces virt-admin client which is based on virsh client, but had to reimplement several methods to meet virt-admin specific needs or remove unnecessary virsh specific logic. --- .gitignore | 1 + daemon/libvirtd.c | 1 - po/POTFILES.in | 1 + tools/Makefile.am | 28 ++- tools/virt-admin.c | 581 +++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virt-admin.h | 47 +++++ 6 files changed, 656 insertions(+), 3 deletions(-) create mode 100644 tools/virt-admin.c create mode 100644 tools/virt-admin.h diff --git a/.gitignore b/.gitignore index 2d52a8f..a776947 100644 --- a/.gitignore +++ b/.gitignore @@ -176,6 +176,7 @@ /tools/virt-login-shell /tools/virsh /tools/virsh-*-edit.c +/tools/virt-admin /tools/virt-*-validate /tools/virt-sanlock-cleanup /tools/wireshark/src/plugin.c diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 250094b..26ccf59 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -522,7 +522,6 @@ daemonSetupNetworking(virNetServerPtr srv, virNetServerAddService(srv, svcRO, NULL) < 0) goto cleanup; - /* Temporarily disabled */ if (sock_path_adm && false) { VIR_DEBUG("Registering unix socket %s", sock_path_adm); if (!(svcAdm = virNetServerServiceNewUNIX(sock_path_adm, diff --git a/po/POTFILES.in b/po/POTFILES.in index 0cc5b99..d0840f4 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -270,6 +270,7 @@ tools/virsh-pool.c tools/virsh-secret.c tools/virsh-snapshot.c tools/virsh-volume.c +tools/virt-admin.c tools/virt-host-validate-common.c tools/virt-host-validate-lxc.c tools/virt-host-validate-qemu.c diff --git a/tools/Makefile.am b/tools/Makefile.am index d5638d9..e68fe84 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -1,4 +1,4 @@ -## Copyright (C) 2005-2014 Red Hat, Inc. +## Copyright (C) 2005-2015 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 @@ -63,7 +63,7 @@ confdir = $(sysconfdir)/libvirt conf_DATA = bin_SCRIPTS = virt-xml-validate virt-pki-validate -bin_PROGRAMS = virsh virt-host-validate +bin_PROGRAMS = virsh virt-host-validate virt-admin libexec_SCRIPTS = libvirt-guests.sh if WITH_SANLOCK @@ -230,6 +230,30 @@ virsh_CFLAGS = \ $(PIE_CFLAGS) \ $(COVERAGE_CFLAGS) \ $(LIBXML_CFLAGS) + +virt_admin_SOURCES = \ + virt-admin.c virt-admin.h \ + $(NULL) + +virt_admin_LDFLAGS = \ + $(AM_LDFLAGS) \ + $(COVERAGE_LDFLAGS) \ + $(NULL) +virt_admin_LDADD = \ + $(STATIC_BINARIES) \ + $(PIE_LDFLAGS) \ + ../src/libvirt.la \ + ../src/libvirt-admin.la \ + ../gnulib/lib/libgnu.la \ + libvirt_shell.la \ + $(LIBXML_LIBS) \ + $(NULL) +virt_admin_CFLAGS = \ + $(WARN_CFLAGS) \ + $(PIE_CFLAGS) \ + $(COVERAGE_CFLAGS) \ + $(LIBXML_CFLAGS) \ + $(READLINE_CFLAGS) BUILT_SOURCES = if WITH_WIN_ICON diff --git a/tools/virt-admin.c b/tools/virt-admin.c new file mode 100644 index 0000000..cc33b7b --- /dev/null +++ b/tools/virt-admin.c @@ -0,0 +1,581 @@ +/* + * virt-admin.c: a shell to exercise the libvirt admin API + * + * Copyright (C) 2015 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, see + * <http://www.gnu.org/licenses/>;. + * + * Erik Skultety <eskultet(a)redhat.com&gt; + */ + +#include <config.h> +#include "virt-admin.h" + +#include <errno.h> +#include <getopt.h> +#include <locale.h> + +#if WITH_READLINE +# include <readline/readline.h> +# include <readline/history.h> +#endif + +#include "internal.h" +#include "virerror.h" +#include "viralloc.h" +#include "virfile.h" +#include "configmake.h" +#include "virthread.h" +#include "virstring.h" + +/* Gnulib doesn't guarantee SA_SIGINFO support. */ +#ifndef SA_SIGINFO +# define SA_SIGINFO 0 +#endif + +#define VIRT_ADMIN_PROMPT "virt-admin # " + +#define vshAdmDisconnect(ctl, conn) vshAdmDisconnectInternal(ctl, &conn); + +static char *progname; + +static const vshCmdGrp cmdGroups[]; +static const vshClientHooks hooks; + +/* + * Detection of disconnections and automatic reconnection support + */ +static bool disconnected; /* we may have been disconnected */ + +static virAdmConnectPtr +vshAdmConnect(vshControl *ctl, unsigned int flags) +{ + vshAdmControlPtr priv = ctl->privData; + + priv->conn = virAdmConnectOpen(ctl->connname, flags); + + if (!priv->conn) { + if (priv->wantReconnect) + vshError(ctl, "%s", _("Failed to reconnect to the admin server")); + else + vshError(ctl, "%s", _("Failed to connect to the admin server")); + return NULL; + } else { + if (priv->wantReconnect) + vshPrint(ctl, "%s\n", _("Reconnected to the admin server")); + else + vshPrint(ctl, "%s\n", _("Connected to the admin server")); + } + + return priv->conn; +} + +static int +vshAdmDisconnectInternal(vshControl *ctl, virAdmConnectPtr *conn) +{ + int ret = 0; + + if (!*conn) + return ret; + + ret = virAdmConnectClose(*conn); + if (ret < 0) + vshError(ctl, "%s", _("Failed to disconnect from the admin server")); + else if (ret > 0) + vshError(ctl, "%s", _("One or more references were leaked after " + "disconnect from the hypervisor")); + *conn = NULL; + return ret; +} + +/* + * vshAdmReconnect: + * + * Reconnect to a daemon's admin server + * + */ +static void +vshAdmReconnect(vshControl *ctl) +{ + vshAdmControlPtr priv = ctl->privData; + if (priv->conn || disconnected) + priv->wantReconnect = true; + + vshAdmDisconnect(ctl, priv->conn); + priv->conn = vshAdmConnect(ctl, 0); + + priv->wantReconnect = false; + disconnected = false; +} + +/* --------------- + * Command Connect + * --------------- + */ + +static const vshCmdOptDef opts_connect[] = { + {.name = "name", + .type = VSH_OT_STRING, + .flags = VSH_OFLAG_EMPTY_OK, + .help = N_("daemon's admin server connection URI") + }, + {.name = NULL} +}; + +static const vshCmdInfo info_connect[] = { + {.name = "help", + .data = N_("(re)connect to daemon's admin server") + }, + {.name = "desc", + .data = N_("(Re)connect to a daemon's administrating server.") + }, + {.name = NULL} +}; + +static bool +cmdConnect(vshControl *ctl, const vshCmd *cmd) +{ + const char *name = NULL; + vshAdmControlPtr priv = ctl->privData; + + VIR_FREE(ctl->connname); + if (vshCommandOptStringReq(ctl, cmd, "name", &name) < 0) + return false; + + ctl->connname = vshStrdup(ctl, name); + + vshAdmReconnect(ctl); + if (!priv->conn) + return false; + + return true; +} + +static void * +vshAdmConnectionHandler(vshControl *ctl) +{ + vshAdmControlPtr priv = ctl->privData; + + if (!priv->conn || disconnected) + vshAdmReconnect(ctl); + + if (!priv->conn) { + vshError(ctl, "%s", _("no valid connection")); + return NULL; + } + + return priv->conn; +} + +/* + * Initialize connection. + */ +static bool +vshAdmInit(vshControl *ctl) +{ + vshAdmControlPtr priv = ctl->privData; + + /* Since we have the commandline arguments parsed, we need to + * reload our initial settings to make debugging and readline + * work properly */ + vshInitReload(ctl); + + if (priv->conn) + return false; + + /* set up the library error handler */ + virSetErrorFunc(NULL, vshErrorHandler); + + if (virEventRegisterDefaultImpl() < 0) + return false; + + if (virThreadCreate(&ctl->eventLoop, true, vshEventLoop, ctl) < 0) + return false; + ctl->eventLoopStarted = true; + + if ((ctl->eventTimerId = virEventAddTimeout(-1, vshEventTimeout, ctl, + NULL)) < 0) + return false; + + if (ctl->connname) { + vshAdmReconnect(ctl); + /* Connecting to a named connection must succeed, but we delay + * connecting to the default connection until we need it + * (since the first command might be 'connect' which allows a + * non-default connection, or might be 'help' which needs no + * connection). + */ + if (!priv->conn) { + vshReportError(ctl); + return false; + } + } + + return true; +} + +static void +vshAdmDeinitTimer(int timer ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) +{ + /* nothing to be done here */ +} + +/* + * Deinitialize virt-admin + */ +static bool +vshAdmDeinit(vshControl *ctl) +{ + vshAdmControlPtr priv = ctl->privData; + + vshDeinit(ctl); + VIR_FREE(ctl->connname); + + if (priv->conn) + vshAdmDisconnect(ctl, priv->conn); + + virResetLastError(); + + if (ctl->eventLoopStarted) { + int timer; + + virMutexLock(&ctl->lock); + ctl->quit = true; + /* HACK: Add a dummy timeout to break event loop */ + timer = virEventAddTimeout(0, vshAdmDeinitTimer, NULL, NULL); + virMutexUnlock(&ctl->lock); + + virThreadJoin(&ctl->eventLoop); + + if (timer != -1) + virEventRemoveTimeout(timer); + + if (ctl->eventTimerId != -1) + virEventRemoveTimeout(ctl->eventTimerId); + + ctl->eventLoopStarted = false; + } + + virMutexDestroy(&ctl->lock); + + return true; +} + +/* + * Print usage + */ +static void +vshAdmUsage(void) +{ + const vshCmdGrp *grp; + const vshCmdDef *cmd; + + fprintf(stdout, _("\n%s [options]... [<command_string>]" + "\n%s [options]... <command> [args...]\n\n" + " options:\n" + " -c | --connect=URI daemon admin connection URI\n" + " -d | --debug=NUM debug level [0-4]\n" + " -h | --help this help\n" + " -l | --log=FILE output logging to file\n" + " -q | --quiet quiet mode\n" + " -v short version\n" + " -V long version\n" + " --version[=TYPE] version, TYPE is short or long (default short)\n" + " commands (non interactive mode):\n\n"), progname, + progname); + + for (grp = cmdGroups; grp->name; grp++) { + fprintf(stdout, _(" %s (help keyword '%s')\n"), + grp->name, grp->keyword); + for (cmd = grp->commands; cmd->name; cmd++) { + if (cmd->flags & VSH_CMD_FLAG_ALIAS) + continue; + fprintf(stdout, + " %-30s %s\n", cmd->name, + _(vshCmddefGetInfo(cmd, "help"))); + } + fprintf(stdout, "\n"); + } + + fprintf(stdout, "%s", + _("\n (specify help <group> for details about the commands in the group)\n")); + fprintf(stdout, "%s", + _("\n (specify help <command> for details about the command)\n\n")); + return; +} + +/* + * Show version and options compiled in + */ +static void +vshAdmShowVersion(vshControl *ctl ATTRIBUTE_UNUSED) +{ + /* FIXME - list a copyright blurb, as in GNU programs? */ + vshPrint(ctl, _("Virt-admin command line tool of libvirt %s\n"), VERSION); + vshPrint(ctl, _("See web site at %s\n\n"), "http://libvirt.org/"); + + vshPrint(ctl, "%s", _("Compiled with support for:\n")); + vshPrint(ctl, "\n"); + vshPrint(ctl, "%s", _(" Miscellaneous:")); +#ifdef WITH_LIBVIRTD + vshPrint(ctl, " Daemon"); +#endif +#ifdef WITH_SECDRIVER_APPARMOR + vshPrint(ctl, " AppArmor"); +#endif +#ifdef WITH_SECDRIVER_SELINUX + vshPrint(ctl, " SELinux"); +#endif +#ifdef ENABLE_DEBUG + vshPrint(ctl, " Debug"); +#endif +#if WITH_READLINE + vshPrint(ctl, " Readline"); +#endif +#ifdef WITH_DRIVER_MODULES + vshPrint(ctl, " Modular"); +#endif + vshPrint(ctl, "\n"); +} + +static bool +vshAdmParseArgv(vshControl *ctl, int argc, char **argv) +{ + int arg, debug; + size_t i; + int longindex = -1; + struct option opt[] = { + {"connect", required_argument, NULL, 'c'}, + {"debug", required_argument, NULL, 'd'}, + {"help", no_argument, NULL, 'h'}, + {"log", required_argument, NULL, 'l'}, + {"quiet", no_argument, NULL, 'q'}, + {"version", optional_argument, NULL, 'v'}, + {NULL, 0, NULL, 0} + }; + + /* Standard (non-command) options. The leading + ensures that no + * argument reordering takes place, so that command options are + * not confused with top-level virt-admin options. */ + while ((arg = getopt_long(argc, argv, "+:c:d:e:h:l:qvV", opt, &longindex)) != -1) { + switch (arg) { + case 'c': + VIR_FREE(ctl->connname); + ctl->connname = vshStrdup(ctl, optarg); + break; + case 'd': + if (virStrToLong_i(optarg, NULL, 10, &debug) < 0) { + vshError(ctl, _("option %s takes a numeric argument"), + longindex == -1 ? "-d" : "--debug"); + exit(EXIT_FAILURE); + } + if (debug < VSH_ERR_DEBUG || debug > VSH_ERR_ERROR) + vshError(ctl, _("ignoring debug level %d out of range [%d-%d]"), + debug, VSH_ERR_DEBUG, VSH_ERR_ERROR); + else + ctl->debug = debug; + break; + case 'h': + vshAdmUsage(); + exit(EXIT_SUCCESS); + break; + case 'l': + vshCloseLogFile(ctl); + ctl->logfile = vshStrdup(ctl, optarg); + vshOpenLogFile(ctl); + break; + case 'q': + ctl->quiet = true; + break; + case 'v': + if (STRNEQ_NULLABLE(optarg, "long")) { + puts(VERSION); + exit(EXIT_SUCCESS); + } + /* fall through */ + case 'V': + vshAdmShowVersion(ctl); + exit(EXIT_SUCCESS); + case ':': + for (i = 0; opt[i].name != NULL; i++) { + if (opt[i].val == optopt) + break; + } + if (opt[i].name) + vshError(ctl, _("option '-%c'/'--%s' requires an argument"), + optopt, opt[i].name); + else + vshError(ctl, _("option '-%c' requires an argument"), optopt); + exit(EXIT_FAILURE); + case '?': + if (optopt) + vshError(ctl, _("unsupported option '-%c'. See --help."), optopt); + else + vshError(ctl, _("unsupported option '%s'. See --help."), argv[optind - 1]); + exit(EXIT_FAILURE); + default: + vshError(ctl, _("unknown option")); + exit(EXIT_FAILURE); + } + longindex = -1; + } + + if (argc == optind) { + ctl->imode = true; + } else { + /* parse command */ + ctl->imode = false; + if (argc - optind == 1) { + vshDebug(ctl, VSH_ERR_INFO, "commands: \"%s\"\n", argv[optind]); + return vshCommandStringParse(ctl, argv[optind]); + } else { + return vshCommandArgvParse(ctl, argc - optind, argv + optind); + } + } + return true; +} + +static const vshCmdDef vshAdmCmds[] = { + VSH_CMD_CD, + VSH_CMD_ECHO, + VSH_CMD_EXIT, + VSH_CMD_HELP, + VSH_CMD_PWD, + VSH_CMD_QUIT, + {.name = "connect", + .handler = cmdConnect, + .opts = opts_connect, + .info = info_connect, + .flags = VSH_CMD_FLAG_NOCONNECT + }, + {.name = NULL} +}; + +static const vshCmdGrp cmdGroups[] = { + {VIRT_ADMIN_CMD_GRP_SHELL, "virt-admin", vshAdmCmds}, + {NULL, NULL, NULL} +}; + +static const vshClientHooks hooks = { + .connHandler = vshAdmConnectionHandler +}; + +int +main(int argc, char **argv) +{ + vshControl _ctl, *ctl = &_ctl; + vshAdmControl virtAdminCtl; + const char *defaultConn; + bool ret = true; + + memset(ctl, 0, sizeof(vshControl)); + memset(&virtAdminCtl, 0, sizeof(vshAdmControl)); + ctl->name = "virt-admin"; /* hardcoded name of the binary */ + ctl->log_fd = -1; /* Initialize log file descriptor */ + ctl->debug = VSH_DEBUG_DEFAULT; + ctl->hooks = &hooks; + + ctl->eventPipe[0] = -1; + ctl->eventPipe[1] = -1; + ctl->eventTimerId = -1; + ctl->privData = &virtAdminCtl; + + if (!(progname = strrchr(argv[0], '/'))) + progname = argv[0]; + else + progname++; + ctl->progname = progname; + + if (!setlocale(LC_ALL, "")) { + perror("setlocale"); + /* failure to setup locale is not fatal */ + } + if (!bindtextdomain(PACKAGE, LOCALEDIR)) { + perror("bindtextdomain"); + return EXIT_FAILURE; + } + if (!textdomain(PACKAGE)) { + perror("textdomain"); + return EXIT_FAILURE; + } + + if (isatty(STDIN_FILENO)) { + ctl->istty = true; + +#ifndef WIN32 + if (tcgetattr(STDIN_FILENO, &ctl->termattr) < 0) + ctl->istty = false; +#endif + } + + if (virMutexInit(&ctl->lock) < 0) { + vshError(ctl, "%s", _("Failed to initialize mutex")); + return EXIT_FAILURE; + } + + if (virInitialize() < 0) { + vshError(ctl, "%s", _("Failed to initialize libvirt")); + return EXIT_FAILURE; + } + + virFileActivateDirOverride(argv[0]); + + if ((defaultConn = virGetEnvBlockSUID("LIBVIRT_DEFAULT_ADMIN_URI"))) + ctl->connname = vshStrdup(ctl, defaultConn); + + if (!vshInit(ctl, cmdGroups, NULL)) + exit(EXIT_FAILURE); + + if (!vshAdmParseArgv(ctl, argc, argv) || + !vshAdmInit(ctl)) { + vshAdmDeinit(ctl); + exit(EXIT_FAILURE); + } + + if (!ctl->imode) { + ret = vshCommandRun(ctl, ctl->cmd); + } else { + /* interactive mode */ + if (!ctl->quiet) { + vshPrint(ctl, + _("Welcome to %s, the administrating virtualization " + "interactive terminal.\n\n"), + progname); + vshPrint(ctl, "%s", + _("Type: 'help' for help with commands\n" + " 'quit' to quit\n\n")); + } + + do { + ctl->cmdstr = vshReadline(ctl, VIRT_ADMIN_PROMPT); + if (ctl->cmdstr == NULL) + break; /* EOF */ + if (*ctl->cmdstr) { +#if WITH_READLINE + add_history(ctl->cmdstr); +#endif + if (vshCommandStringParse(ctl, ctl->cmdstr)) + vshCommandRun(ctl, ctl->cmd); + } + VIR_FREE(ctl->cmdstr); + } while (ctl->imode); + + if (ctl->cmdstr == NULL) + fputc('\n', stdout); /* line break after alone prompt */ + } + + vshAdmDeinit(ctl); + exit(ret ? EXIT_SUCCESS : EXIT_FAILURE); +} diff --git a/tools/virt-admin.h b/tools/virt-admin.h new file mode 100644 index 0000000..f7a8931 --- /dev/null +++ b/tools/virt-admin.h @@ -0,0 +1,47 @@ +/* + * virt-admin.h: a shell to exercise the libvirt admin API + * + * Copyright (C) 2015 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, see + * <http://www.gnu.org/licenses/>;. + * + * Erik Skultety <eskultet(a)redhat.com&gt; + */ + +#ifndef VIRT_ADMIN_H +# define VIRT_ADMIN_H + +# include "internal.h" +# include "vsh.h" + +# define VIR_FROM_THIS VIR_FROM_NONE + +/* + * Command group types + */ +# define VIRT_ADMIN_CMD_GRP_SHELL "Virt-admin itself" + +typedef struct _vshAdmControl vshAdmControl; +typedef vshAdmControl *vshAdmControlPtr; + +/* + * adminControl + */ +struct _vshAdmControl { + virAdmConnectPtr conn; /* connection to a daemon's admin server */ + bool wantReconnect; +}; + +#endif /* VIRT_ADMIN_H */ -- 2.4.3

Since most of our APIs rely on an acive functional connection to a daemon and we have such a mechanism in libvirt already, there's need to have such a way in libvirt-admin as well. By introducing a new public API, this patch provides support to check for an active connection. --- include/libvirt/libvirt-admin.h | 1 + src/libvirt-admin.c | 31 +++++++++++++++++++++++++++++++ src/libvirt_admin_public.syms | 1 + tools/virt-admin.c | 2 +- 4 files changed, 34 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h index 9997cc2..72671c6 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -52,6 +52,7 @@ virAdmConnectPtr virAdmConnectOpen(const char *name, unsigned int flags); int virAdmConnectClose(virAdmConnectPtr conn); int virAdmConnectRef(virAdmConnectPtr conn); +int virAdmConnectIsAlive(virAdmConnectPtr conn); # ifdef __cplusplus } diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index 5a4fc48..f0824dd 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -386,3 +386,34 @@ virAdmConnectRef(virAdmConnectPtr conn) return 0; } + +/** + * virAdmConnectIsAlive: + * @conn: connection to admin server + * + * Decide whether the connection to the admin server is alive or not. + * Connection is considered alive if the channel it is running over is not + * closed. + * + * Returns 1, if the connection is alive, 0 if the channel has already + * been closed. + */ +int +virAdmConnectIsAlive(virAdmConnectPtr conn) +{ + bool ret; + remoteAdminPrivPtr priv = conn->privateData; + + VIR_DEBUG("conn=%p", conn); + + virResetLastError(); + + virObjectLock(priv); + ret = virNetClientIsOpen(priv->client); + virObjectUnlock(priv); + + if (ret) + return 1; + else + return 0; +} diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index d9e3c0b..16cfd42 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -15,4 +15,5 @@ LIBVIRT_ADMIN_1.3.0 { virAdmConnectOpen; virAdmConnectClose; virAdmConnectRef; + virAdmConnectIsAlive; }; diff --git a/tools/virt-admin.c b/tools/virt-admin.c index cc33b7b..1bc10b0 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -171,7 +171,7 @@ vshAdmConnectionHandler(vshControl *ctl) if (!priv->conn || disconnected) vshAdmReconnect(ctl); - if (!priv->conn) { + if (!priv->conn || !virAdmConnectIsAlive(priv->conn)) { vshError(ctl, "%s", _("no valid connection")); return NULL; } -- 2.4.3

Yep, thanks. Well, you're right. But things are more interesting than that. If you look at it closely (and I must have been blind or something until now), disconnect and virConnectIsAlive (standard libvirt) symbolize the very same thing, so the presence of disconnect variable in virsh is imho due to historical reasons (as virConnectIsAlive has been present since 0.9.X). In virt-admin however, virAdmConnectIsAlive API will be introduced at the same time as the client itself, so having that variable in the code will only contribute to more confusion. And I also need to have a look at vsh module which I added recently as 'disconnected' sneaked into that one as well and I'm not sure yet it is necessary to have it there at all.

By moving the remote version into a separate module, we gain a slightly better maintainability in the long run than just by leaving it in one place with the existing libvirt-admin library which can start getting pretty messy later on. --- src/admin/admin_remote.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++ src/libvirt-admin.c | 115 +-------------------------------------- 2 files changed, 137 insertions(+), 114 deletions(-) create mode 100644 src/admin/admin_remote.c diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c new file mode 100644 index 0000000..b8e6607 --- /dev/null +++ b/src/admin/admin_remote.c @@ -0,0 +1,136 @@ +/* + * admin_remote.c + * + * Copyright (C) 2015 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, see + * <http://www.gnu.org/licenses/>;. + * + * Author: Erik Skultety <eskultet(a)redhat.com&gt; + */ + +#include <config.h> +#include <rpc/rpc.h> +#include "admin_protocol.h" + +typedef struct _remoteAdminPriv remoteAdminPriv; +typedef remoteAdminPriv *remoteAdminPrivPtr; + +struct _remoteAdminPriv { + virObjectLockable parent; + + int counter; + virNetClientPtr client; + virNetClientProgramPtr program; +}; + +static virClassPtr remoteAdminPrivClass; + +static void +remoteAdminPrivDispose(void *opaque) +{ + remoteAdminPrivPtr priv = opaque; + + virObjectUnref(priv->program); + virObjectUnref(priv->client); +} + + +static int +callFull(virAdmConnectPtr conn ATTRIBUTE_UNUSED, + remoteAdminPrivPtr priv, + int *fdin, + size_t fdinlen, + int **fdout, + size_t *fdoutlen, + int proc_nr, + xdrproc_t args_filter, char *args, + xdrproc_t ret_filter, char *ret) +{ + int rv; + virNetClientProgramPtr prog = priv->program; + int counter = priv->counter++; + virNetClientPtr client = priv->client; + + /* Unlock, so that if we get any async events/stream data + * while processing the RPC, we don't deadlock when our + * callbacks for those are invoked + */ + virObjectRef(priv); + virObjectUnlock(priv); + + rv = virNetClientProgramCall(prog, + client, + counter, + proc_nr, + fdinlen, fdin, + fdoutlen, fdout, + args_filter, args, + ret_filter, ret); + + virObjectLock(priv); + virObjectUnref(priv); + + return rv; +} + +static int +call(virAdmConnectPtr conn, + unsigned int flags, + int proc_nr, + xdrproc_t args_filter, char *args, + xdrproc_t ret_filter, char *ret) +{ + virCheckFlags(0, -1); + + return callFull(conn, conn->privateData, + NULL, 0, NULL, NULL, proc_nr, + args_filter, args, ret_filter, ret); +} + +#include "admin_client.h" + +static void +remoteAdminPrivFree(void *opaque) +{ + virAdmConnectPtr conn = opaque; + + remoteAdminConnectClose(conn); + virObjectUnref(conn->privateData); +} + +static remoteAdminPrivPtr +remoteAdminPrivNew(const char *sock_path) +{ + remoteAdminPrivPtr priv = NULL; + + if (!(priv = virObjectLockableNew(remoteAdminPrivClass))) + goto error; + + if (!(priv->client = virNetClientNewUNIX(sock_path, false, NULL))) + goto error; + + if (!(priv->program = virNetClientProgramNew(ADMIN_PROGRAM, + ADMIN_PROTOCOL_VERSION, + NULL, 0, NULL))) + goto error; + + if (virNetClientAddProgram(priv->client, priv->program) < 0) + goto error; + + return priv; + error: + virObjectUnref(priv); + return NULL; +} diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index f0824dd..cdc712c 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -22,8 +22,6 @@ #include <config.h> -#include <rpc/rpc.h> - #include "internal.h" #include "datatypes.h" #include "configmake.h" @@ -44,121 +42,10 @@ VIR_LOG_INIT("libvirt-admin"); - -typedef struct _remoteAdminPriv remoteAdminPriv; -typedef remoteAdminPriv *remoteAdminPrivPtr; - -struct _remoteAdminPriv { - virObjectLockable parent; - - int counter; - virNetClientPtr client; - virNetClientProgramPtr program; -}; - -static virClassPtr remoteAdminPrivClass; - -static void -remoteAdminPrivDispose(void *opaque) -{ - remoteAdminPrivPtr priv = opaque; - - virObjectUnref(priv->program); - virObjectUnref(priv->client); -} - - -static int -callFull(virAdmConnectPtr conn ATTRIBUTE_UNUSED, - remoteAdminPrivPtr priv, - int *fdin, - size_t fdinlen, - int **fdout, - size_t *fdoutlen, - int proc_nr, - xdrproc_t args_filter, char *args, - xdrproc_t ret_filter, char *ret) -{ - int rv; - virNetClientProgramPtr prog = priv->program; - int counter = priv->counter++; - virNetClientPtr client = priv->client; - - /* Unlock, so that if we get any async events/stream data - * while processing the RPC, we don't deadlock when our - * callbacks for those are invoked - */ - virObjectRef(priv); - virObjectUnlock(priv); - - rv = virNetClientProgramCall(prog, - client, - counter, - proc_nr, - fdinlen, fdin, - fdoutlen, fdout, - args_filter, args, - ret_filter, ret); - - virObjectLock(priv); - virObjectUnref(priv); - - return rv; -} - -static int -call(virAdmConnectPtr conn, - unsigned int flags, - int proc_nr, - xdrproc_t args_filter, char *args, - xdrproc_t ret_filter, char *ret) -{ - virCheckFlags(0, -1); - - return callFull(conn, conn->privateData, - NULL, 0, NULL, NULL, proc_nr, - args_filter, args, ret_filter, ret); -} - -#include "admin_protocol.h" -#include "admin_client.h" - static bool virAdmGlobalError; static virOnceControl virAdmGlobalOnce = VIR_ONCE_CONTROL_INITIALIZER; -static void -remoteAdminPrivFree(void *opaque) -{ - virAdmConnectPtr conn = opaque; - - remoteAdminConnectClose(conn); - virObjectUnref(conn->privateData); -} - -static remoteAdminPrivPtr -remoteAdminPrivNew(const char *sock_path) -{ - remoteAdminPrivPtr priv = NULL; - - if (!(priv = virObjectLockableNew(remoteAdminPrivClass))) - goto error; - - if (!(priv->client = virNetClientNewUNIX(sock_path, false, NULL))) - goto error; - - if (!(priv->program = virNetClientProgramNew(ADMIN_PROGRAM, - ADMIN_PROTOCOL_VERSION, - NULL, 0, NULL))) - goto error; - - if (virNetClientAddProgram(priv->client, priv->program) < 0) - goto error; - - return priv; - error: - virObjectUnref(priv); - return NULL; -} +#include "admin_remote.c" static void virAdmGlobalInit(void) -- 2.4.3

As we plan to add more and more logic to remote connecting methods, these cannot be generated from admin_protocol.x anymore. Instead, this patch implements these to methods explicitly. --- src/admin/admin_protocol.x | 4 ++-- src/admin/admin_remote.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/admin/admin_protocol.x b/src/admin/admin_protocol.x index cfc92ff..d0ca1a3 100644 --- a/src/admin/admin_protocol.x +++ b/src/admin/admin_protocol.x @@ -64,12 +64,12 @@ enum admin_procedure { * in the function parameter list. */ /** - * @generate: client + * @generate: none */ ADMIN_PROC_CONNECT_OPEN = 1, /** - * @generate: client + * @generate: none */ ADMIN_PROC_CONNECT_CLOSE = 2 }; diff --git a/src/admin/admin_remote.c b/src/admin/admin_remote.c index b8e6607..2c02ba6 100644 --- a/src/admin/admin_remote.c +++ b/src/admin/admin_remote.c @@ -101,6 +101,51 @@ call(virAdmConnectPtr conn, #include "admin_client.h" +static int +remoteAdminConnectOpen(virAdmConnectPtr conn, unsigned int flags) +{ + int rv = -1; + remoteAdminPrivPtr priv = conn->privateData; + admin_connect_open_args args; + + virObjectLock(priv); + + args.flags = flags; + + if (call(conn, 0, ADMIN_PROC_CONNECT_OPEN, + (xdrproc_t)xdr_admin_connect_open_args, (char *)&args, + (xdrproc_t)xdr_void, (char *)NULL) == -1) { + goto done; + } + + rv = 0; + + done: + virObjectUnlock(priv); + return rv; +} + +static int +remoteAdminConnectClose(virAdmConnectPtr conn) +{ + int rv = -1; + remoteAdminPrivPtr priv = conn->privateData; + + virObjectLock(priv); + + if (call(conn, 0, ADMIN_PROC_CONNECT_CLOSE, + (xdrproc_t)xdr_void, (char *)NULL, + (xdrproc_t)xdr_void, (char *)NULL) == -1) { + goto done; + } + + rv = 0; + + done: + virObjectUnlock(priv); + return rv; +} + static void remoteAdminPrivFree(void *opaque) { -- 2.4.3

Since virt-admin should be able to connect to various admin servers on hosted different daemons, we need to provide URI support to libvirt-admin. --- include/libvirt/libvirt-admin.h | 2 + src/datatypes.c | 2 + src/datatypes.h | 1 + src/libvirt-admin.c | 132 +++++++++++++++++++++++++++++++--------- src/libvirt_admin_public.syms | 1 + tools/virt-admin.c | 39 ++++++++++++ 6 files changed, 147 insertions(+), 30 deletions(-) diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h index 72671c6..aae10b4 100644 --- a/include/libvirt/libvirt-admin.h +++ b/include/libvirt/libvirt-admin.h @@ -54,6 +54,8 @@ int virAdmConnectClose(virAdmConnectPtr conn); int virAdmConnectRef(virAdmConnectPtr conn); int virAdmConnectIsAlive(virAdmConnectPtr conn); +char * virAdmConnectGetURI(virAdmConnectPtr conn); + # ifdef __cplusplus } # endif diff --git a/src/datatypes.c b/src/datatypes.c index 12bcfc1..aeac301 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -832,4 +832,6 @@ virAdmConnectDispose(void *obj) if (conn->privateDataFreeFunc) conn->privateDataFreeFunc(conn); + + virURIFree(conn->uri); } diff --git a/src/datatypes.h b/src/datatypes.h index be108fe..95aa49e 100644 --- a/src/datatypes.h +++ b/src/datatypes.h @@ -397,6 +397,7 @@ struct _virConnect { */ struct _virAdmConnect { virObjectLockable object; + virURIPtr uri; void *privateData; virFreeCallback privateDataFreeFunc; diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c index cdc712c..9fc8393 100644 --- a/src/libvirt-admin.c +++ b/src/libvirt-admin.c @@ -27,6 +27,7 @@ #include "configmake.h" #include "viralloc.h" +#include "virconf.h" #include "virlog.h" #include "virnetclient.h" #include "virobject.h" @@ -95,60 +96,54 @@ virAdmInitialize(void) } static char * -getSocketPath(const char *name) +getSocketPath(virURIPtr uri) { char *rundir = virGetUserRuntimeDirectory(); char *sock_path = NULL; size_t i = 0; - virURIPtr uri = NULL; - if (name) { - if (!(uri = virURIParse(name))) - goto error; + if (!uri) + goto cleanup; - if (STRNEQ(uri->scheme, "admin") || - uri->server || uri->user || uri->fragment) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid connection name '%s'"), name); - goto error; - } - for (i = 0; i < uri->paramsCount; i++) { - virURIParamPtr param = &uri->params[i]; + for (i = 0; i < uri->paramsCount; i++) { + virURIParamPtr param = &uri->params[i]; - if (STREQ(param->name, "socket")) { - VIR_FREE(sock_path); - if (VIR_STRDUP(sock_path, param->value) < 0) - goto error; - } else { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unknown URI parameter '%s'"), param->name); + if (STREQ(param->name, "socket")) { + VIR_FREE(sock_path); + if (VIR_STRDUP(sock_path, param->value) < 0) goto error; - } + } else { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unknown URI parameter '%s'"), param->name); + goto error; } } if (!sock_path) { - if (!uri || !uri->path || STREQ(uri->path, "/system")) { + if (STRNEQ(uri->scheme, "libvirtd")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported URI scheme '%s'"), + uri->scheme); + goto error; + } + if (STREQ_NULLABLE(uri->path, "/system")) { if (VIR_STRDUP(sock_path, LIBVIRTD_ADMIN_UNIX_SOCKET) < 0) goto error; } else if (STREQ_NULLABLE(uri->path, "/session")) { - if (!rundir) - goto error; - - if (virAsprintf(&sock_path, - "%s%s", rundir, LIBVIRTD_ADMIN_SOCK_NAME) < 0) + if (!rundir && virAsprintf(&sock_path, "%s%s", rundir, + LIBVIRTD_ADMIN_SOCK_NAME) < 0) goto error; } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid URI path '%s'"), uri->path); + _("Invalid URI path '%s', try '/system'"), + uri->path ? uri->path : ""); goto error; } } cleanup: VIR_FREE(rundir); - virURIFree(uri); return sock_path; error: @@ -156,6 +151,40 @@ getSocketPath(const char *name) goto cleanup; } +static int +virAdmGetDefaultURI(virConfPtr conf, virURIPtr *uri) +{ + virConfValuePtr value = NULL; + const char *uristr = NULL; + + uristr = virGetEnvBlockSUID("LIBVIRT_ADMIN_DEFAULT_URI"); + if (uristr && *uristr) { + VIR_DEBUG("Using LIBVIRT_ADMIN_DEFAULT_URI '%s'", uristr); + } else if ((value = virConfGetValue(conf, "uri_default_admin"))) { + if (value->type != VIR_CONF_STRING) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Expected a string for 'uri_default_admin' config " + "parameter")); + return -1; + } + + VIR_DEBUG("Using config file uri '%s'", value->str); + uristr = value->str; + } else { + /* Since we can't probe connecting via any hypervisor driver as libvirt + * does, if no explicit URI was given and neither the environment + * variable, nor the configuration parameter had previously been set, + * we set the default admin server URI to 'libvirtd://system'. + */ + uristr = "libvirtd:///system"; + } + + if (!(*uri = virURIParse(uristr))) + return -1; + + return 0; +} + /** * virAdmConnectOpen: * @name: uri of the daemon to connect to, NULL for default @@ -170,6 +199,7 @@ virAdmConnectOpen(const char *name, unsigned int flags) { char *sock_path = NULL; virAdmConnectPtr conn = NULL; + virConfPtr conf = NULL; if (virAdmInitialize() < 0) goto error; @@ -180,7 +210,18 @@ virAdmConnectOpen(const char *name, unsigned int flags) if (!(conn = virAdmConnectNew())) goto error; - if (!(sock_path = getSocketPath(name))) + if (virGetLibvirtConfigFile(&conf) < 0) + goto error; + + if (!name) { + if (virAdmGetDefaultURI(conf, &conn->uri) < 0) + goto error; + } else { + if (!(conn->uri = virURIParse(name))) + goto error; + } + + if (!(sock_path = getSocketPath(conn->uri))) goto error; if (!(conn->privateData = remoteAdminPrivNew(sock_path))) @@ -304,3 +345,34 @@ virAdmConnectIsAlive(virAdmConnectPtr conn) else return 0; } + +/** + * virAdmConnectGetURI: + * @conn: pointer to an admin connection + * + * String returned by this method is normally the same as the string passed + * to the virAdmConnectOpen. Even if NULL was passed to virAdmConnectOpen, + * this method returns a non-null URI string. + * + * Returns an URI string related to the connection or NULL in case of an error. + * Caller is responsible for freeing the string. + */ +char * +virAdmConnectGetURI(virAdmConnectPtr conn) +{ + char *uri = NULL; + VIR_DEBUG("conn=%p", conn); + + virResetLastError(); + + virCheckAdmConnectReturn(conn, NULL); + + if (!(uri = virURIFormat(conn->uri))) + goto error; + + return uri; + + error: + virDispatchError(NULL); + return uri; +} diff --git a/src/libvirt_admin_public.syms b/src/libvirt_admin_public.syms index 16cfd42..069508c 100644 --- a/src/libvirt_admin_public.syms +++ b/src/libvirt_admin_public.syms @@ -16,4 +16,5 @@ LIBVIRT_ADMIN_1.3.0 { virAdmConnectClose; virAdmConnectRef; virAdmConnectIsAlive; + virAdmConnectGetURI; }; diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 1bc10b0..c00a3b3 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -120,6 +120,39 @@ vshAdmReconnect(vshControl *ctl) disconnected = false; } +/* + * 'uri' command + */ + +static const vshCmdInfo info_uri[] = { + {.name = "help", + .data = N_("print the admin server URI") + }, + {.name = "desc", + .data = "" + }, + {.name = NULL} +}; + +static bool +cmdURI(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) +{ + char *uri; + vshAdmControlPtr priv = ctl->privData; + + uri = virAdmConnectGetURI(priv->conn); + if (!uri) { + vshError(ctl, "%s", _("failed to get URI")); + return false; + } + + vshPrint(ctl, "%s\n", uri); + VIR_FREE(uri); + + return true; +} + + /* --------------- * Command Connect * --------------- @@ -454,6 +487,12 @@ static const vshCmdDef vshAdmCmds[] = { VSH_CMD_HELP, VSH_CMD_PWD, VSH_CMD_QUIT, + {.name = "uri", + .handler = cmdURI, + .opts = NULL, + .info = info_uri, + .flags = 0 + }, {.name = "connect", .handler = cmdConnect, .opts = opts_connect, -- 2.4.3

Actually, yes. A couple of days ago I realized that I forgot to introduce URI aliases support in this series, so I prepared a patch for that on my local branch, where I'm reusing the @conf reference as I need to read uri_aliases variable from the config.

On 05/11/15 00:11, John Ferlan wrote: Luckily, I figured that out when I was playing with URI aliases support (coming in v3), but still, thank you very much for noticing :). Erik

On Fri, Oct 16, 2015 at 08:12:24PM +0200, Erik Skultety wrote: Again, yes, we have this code in place already, in the remote driver, and I understand it actually means RemoveCloseCallback, but why aren't we passing NULL as all three parameters after the client? I'd add one empty line here. Otherwise looks fine to me (with the adjustments that will be needed after previous patches are fixed, of course). Martin

Unlike 6/7, I haven't noticed this one, so again, thanks :). Erik

On Fri, Oct 16, 2015 at 08:12:25PM +0200, Erik Skultety wrote: I wanted to differentiate two different calls with admCall and adminCall, I wish I remembered which were those. Otherwise looks fine to me.

On Fri, Oct 16, 2015 at 08:12:26PM +0200, Erik Skultety wrote: If you do s/virt-admin/%, then you can make it a general rule and clean up the makefile a little bit. This reads weirdly. I would say either "user interface for daemon administration" or "daemon administration interface" or something similar. Modify and tweak have the same meaning here. Not only libvirt, it will be enabled for other daemons as well. And it will do more, how about abstracting it a bit, e.g.: The B<virt-admin> program is the main administrating interface for modifying daemon configuration in runtime as well as monitoring and changing daemon behaviour. or something along those lines. s/VIRSH_DEBUG/VIRT_ADMIN_DEBUG/ and also add that info here (copy-paste with name changes from virsh.pod is fine) While on this, I've found out that if I run 'virt-admin -h' it errors out with: error: option '-h'/'--help' requires an argument but 'virt-admin --help' works fine. One of these "extra informational messages" is newline after any command. It can be seen clearly with 'echo asdf' run in virt-admin -q and without -q :) Unfortunately, virsh does that too. Although this is nice to say, I don't think we can do that. From the point when we enable it, it must be backward compatible. We can break compatibility only until we remove that ' && false'. I would either mention other services since it can be said to connect somewhere else. Or not mention this at all. If people run this command, they most probably have daemon running and want to do something to it. I wouldn't mention the group privilege, or add something similar to what's mentioned before the unix_sock_admin_perms config option. We also print out minor and micro version. not necessarily libvirt, also I would not completely error out when connection to daemon cannot be established. Also we throw too many errors in that case, look at it: error: Failed to connect to the admin server error: no valid connection error: Failed to connect socket to '/var/run/libvirt/libvirt-admin-sock': No such file or directory That's pretty confusing for the user, well, mainly for users who don't read their error messages if they have more than one line =D We should clear up the vshError() calls. My reasoning for this is that we might ask someone for the version info and use that for debugging, so the more we can print the better, but even something small can help. These are all the things we will have to go through and clean up later after we turn it on for other daemons. there is no edit option, and we don't know if there will be, just generelize it (like it will be used when running an editor will be needed or something This is more probably based on the virsh man page, but that's just my guess ;) virt-top, virt-df and virt-install are not that relevant here also consider adding virt-host-validate (what if we add support for admin interface checking there).