On Fri, Oct 16, 2015 at 08:12:18PM +0200, Erik Skultety wrote:

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_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;

I would name it virConf... to go with the rest of the file. How about virConfLoad() or virConfLoadDefault(). We don't necessarily need over 20 characters long names for each function in our source =D

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__ */

Keep the space between the declaration and the #endif. ACK with those things fixed. Martin

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@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@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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Oct 16, 2015 at 08:12:20PM +0200, Erik Skultety wrote:

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;

return ret; is fine here.

+} 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)) {

I know we don't do that in virsh, so it doesn't seem obvious here, but shouldn't we do reconnect also when the connection is not alive? I, personally, would add it into the previous condition as well. ACK.

vshError(ctl, "%s", _("no valid connection")); return NULL; } -- 2.4.3

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Nov 04, 2015 at 01:56:40PM +0100, Erik Skultety wrote:

...

...

+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;

return ret; is fine here.

Yep, thanks.

...

...

+} 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)) {

I know we don't do that in virsh, so it doesn't seem obvious here, but shouldn't we do reconnect also when the connection is not alive? I, personally, would add it into the previous condition as well.

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.

Oh, cool, I didn't go to such details. It would certainly be nice to have this cleaned up.

...

ACK.

...

vshError(ctl, "%s", _("no valid connection")); return NULL; } -- 2.4.3

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Oct 16, 2015 at 08:12:21PM +0200, Erik Skultety wrote:

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@redhat.com> + */ + +#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;

[...] many lines [...]

+#include "admin_remote.c"

Simple code movement, ACK, although I would just move the include so that the virAdmGlobal* things are together. I have no idea why I had them so far apart... Martin

static void virAdmGlobalInit(void) -- 2.4.3

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Oct 16, 2015 at 08:12:23PM +0200, Erik Skultety wrote:

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);

extra space after asterisk

+ # 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;

extra whitespace

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;

This is the reason why I kept the conditions separated. So it's clear to read. You are changing the logic here, what you meant is to use || instead of &&. Either fix it to || or leave it unchanged.

} 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");

I don't see the point for the 'BlockSUID()' version, did I miss something?

+ if (uristr && *uristr) { + VIR_DEBUG("Using LIBVIRT_ADMIN_DEFAULT_URI '%s'", uristr); + } else if ((value = virConfGetValue(conf, "uri_default_admin"))) {

I would use 'admin' as a prefix, so we can differentiate any future config options. So either uri_admin_default or admin_uri_default.

+ 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)

Are we planning on using the @conf anywhere else? If not, I'd put the config getting inside this function.

+ goto error; + } else { + if (!(conn->uri = virURIParse(name)))

Change @name to @uri, so it's clear what it is.

+ 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;

You could save some lines with: if (!(uri = virURIFormat(conn->uri))) virDispatchError(NULL); return uri; But whatever flows your boat.

+} 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

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 10/16/2015 02:12 PM, Erik Skultety wrote:

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(-)

Ran the series through the Coverity checks... [...]

/** * 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; +

conf is allocated now and will need to be free'd/cleaned appropriately via virConfFree() @ error: John

+ 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; +}

[...]

On 10/16/2015 02:12 PM, Erik Skultety wrote:

As we need a client disconnect handler, we also need a mechanism to register such handlers for a client. This patch introduced both the close callbacks and also the client vshAdmCatchDisconnect handler to be registered with it. By registering the handler we still need to make sure the client can react to daemon's events like disconnect or keepalive, so asynchronous I/O event polling is necessary to be enabled too. --- include/libvirt/libvirt-admin.h | 19 +++++++ src/admin/admin_remote.c | 36 +++++++++++++ src/datatypes.c | 20 +++++++ src/datatypes.h | 14 ++++- src/libvirt-admin.c | 112 ++++++++++++++++++++++++++++++++++++++++ src/libvirt_admin_public.syms | 2 + tools/virt-admin.c | 50 ++++++++++++++++++ 7 files changed, 252 insertions(+), 1 deletion(-)

Ran the series through the Coverity checks... [...]

diff --git a/src/datatypes.c b/src/datatypes.c index aeac301..603b168 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -60,8 +60,10 @@ static void virStorageVolDispose(void *obj); static void virStoragePoolDispose(void *obj);

virClassPtr virAdmConnectClass; +virClassPtr virAdmConnectCloseCallbackDataClass;

static void virAdmConnectDispose(void *obj); +static void virAdmConnectCloseCallbackDataDispose(void *obj);

static int virDataTypesOnceInit(void) @@ -91,6 +93,7 @@ virDataTypesOnceInit(void) DECLARE_CLASS(virStoragePool);

DECLARE_CLASS_LOCKABLE(virAdmConnect); + DECLARE_CLASS_LOCKABLE(virAdmConnectCloseCallbackData);

#undef DECLARE_CLASS_COMMON #undef DECLARE_CLASS_LOCKABLE @@ -822,6 +825,9 @@ virAdmConnectNew(void) if (!(ret = virObjectLockableNew(virAdmConnectClass))) return NULL;

+ if (!(ret->closeCallback = virObjectLockableNew(virAdmConnectCloseCallbackDataClass))) + return NULL; +

You'll need to free ret or have some sort of cleanup here. John

return ret; }

@@ -834,4 +840,18 @@ virAdmConnectDispose(void *obj) conn->privateDataFreeFunc(conn);

virURIFree(conn->uri); + virObjectUnref(conn->closeCallback); +} + +static void +virAdmConnectCloseCallbackDataDispose(void *obj) +{ + virAdmConnectCloseCallbackDataPtr cb_data = obj; + + virObjectLock(cb_data); + + if (cb_data->freeCallback) + cb_data->freeCallback(cb_data->opaque); + + virObjectUnlock(cb_data); }

[...]