[libvirt] [PATCH v2 2/9] virt-admin: Introduce first working skeleton
Martin Kletzander
mkletzan at redhat.com
Mon Oct 19 15:14:23 UTC 2015
On Fri, Oct 16, 2015 at 08:12:19PM +0200, Erik Skultety wrote:
>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 */
Haha, still not cleaned up properly =)
> 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 = \
misaligned backslashes
>+ $(AM_LDFLAGS) \
>+ $(COVERAGE_LDFLAGS) \
>+ $(NULL)
>+virt_admin_LDADD = \
>+ $(STATIC_BINARIES) \
>+ $(PIE_LDFLAGS) \
I believe this should be in LDFLAGS, I see this is copy-paste from
virsh that has it wrong as well. Well, not wrong as in "it won't
work", but confusing.
>+ ../src/libvirt.la \
>+ ../src/libvirt-admin.la \
>+ ../gnulib/lib/libgnu.la \
Another set of unaligned backslashes, plus there is no need for
libvirt.la (as that is prerequisite of libvirt-admin.la) and neither
for libgnu.la since that is BUILT_LIBADD into libvirt.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 at redhat.com>
>+ */
>+
>+#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"
If nitpicking, I'd sort these more so the vir* files are next to each
other.
>+#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);
>+
Don't do this. It looks like it won't modify the connect pointer and
it's a macro even though it doesn't look like it, etc. Either call it
directly with &priv->conn or just call it with priv if you don't like
the dereference. You can then also remove the 'Internal' naming.
...
Coming back after going through the file, why does it even have the
second parameter? Are we going to connect to multiple admin servers
like we do in virsh for migrations? That could clear few things up.
The vshAdmConnect() function works like this already.
>+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 */
>+
Wow, have we. Maybe two more comments to make sure what this bool
does? :-) Don't take it the wrong way, it just made me laugh.
Anyway, it's a bit confusing to add variable here that you never set
to anything until later when you introduce the disconnection event.
I'm not sure why we have the 'disconnected' variable defined as int
normally, maybe just to be sure that when doing disconnected++ it
won't be stored into something that would overflow easily, but I think
we can count on the fact that it will be stored in int anyway. And,
mainly, Michal asked for it specifically in the first version, IIRC,
so let's leave it like this while keeping in mind that in case this
breaks something, it's his fault ;)
>+static virAdmConnectPtr
>+vshAdmConnect(vshControl *ctl, unsigned int flags)
>+{
>+ vshAdmControlPtr priv = ctl->privData;
>+
>+ priv->conn = virAdmConnectOpen(ctl->connname, flags);
>+
You're setting the pointer here, then returning it and then setting
hte same place to the return value of this function. Is there a
reason for that that I missed? Because otherwise it doesn't make
sense.
>+ 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);
>+
This is what I was referencing from above with the dual assignment.
>+ 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.")
This reads weird, but after reading what we say in virsh.c, it's still
way better =)
>+ },
>+ {.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;
>+
OK, I see this is in virsh, too, but I wonder whether the problem I
see here can cause some trouble. If vshCommandOptStringReq() fails,
we don't connect anywhere, but the previous connname is cleared by the
VIR_FREE(), so later on if reconnect needs to be done, it will connect
somewhere else. Super-corner-case, but still...
>+ ctl->connname = vshStrdup(ctl, name);
>+
>+ vshAdmReconnect(ctl);
>+ if (!priv->conn)
>+ return false;
>+
>+ return true;
Another part that's confusing. I though I knew why this is not
'return priv->conn', but you don't register the close callback here
even in later patches, so I think it could directly return priv->conn
as a bool (or !!priv->conn if you dislike the former).
>+}
>+
>+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");
Well, this is bogus, ain't it?
>+ 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");
Not needed either. Well, most of this is useless, I guess.
>+#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) {
You want to call virAdmInitialize() here, I believe.
>+ 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 at redhat.com>
>+ */
>+
>+#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"
>+
Does this need to be defined at all? I don't know what the origin of
the _CMD_GRP_ macros is, but it sure doesn't make sense for me when
it's not used anywhere.
>+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
>
Most of what I commented on are no deal breakers, just nice-to-have-in
things. I'll continue the review tomorrow, but that shouldn't keep
anyone else from looking into it as well ;)
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151019/27647315/attachment-0001.sig>
More information about the libvir-list
mailing list