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@xxxxxxxxxx> + */ + +#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@xxxxxxxxxx>
+ */
+
+#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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list