[libvirt] [PATCH 0/2] tools: Fix usage of incorrect environment variables
According to https://bugzilla.redhat.com/show_bug.cgi?id=1357363, most of virt-admin's environment variables are not taken into effect when set. As it turned out, virsh suffers from the very same problem which resides in the generic vsh.c module. Erik Skultety (2): vsh: Make vshInitDebug return int instead of void tools: Make use of the correct environment variables tools/virsh.c | 1 + tools/virt-admin.c | 1 + tools/vsh.c | 34 +++++++++++++++++++--------------- tools/vsh.h | 1 + 4 files changed, 22 insertions(+), 15 deletions(-) -- 2.5.5
Well, the reason behind this change is that if the function is extended in some way that e.g. would involve allocation we do not have a way of telling it to the caller. More specifically, vshInitDebug only relies on some hardcoded environment variables (by a mistake) that aren't documented anywhere so neither virsh's nor virt-admin's documented environment variables take effect. One possible solution would be duplicate the code for each CLI client or leave the method be generic and provide means that it could figure out, which client called it, thus initializing the proper environment variables but that could involve operations that might as well fail in certain circumstances and the caller should know that an error occurred. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/vsh.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 9ac4f21..65bd9a4 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2966,7 +2966,7 @@ vshReadline(vshControl *ctl, const char *prompt) /* * Initialize debug settings. */ -static void +static int vshInitDebug(vshControl *ctl) { const char *debugEnv; @@ -2994,6 +2994,8 @@ vshInitDebug(vshControl *ctl) vshOpenLogFile(ctl); } } + + return 0; } @@ -3016,9 +3018,9 @@ vshInit(vshControl *ctl, const vshCmdGrp *groups, const vshCmdDef *set) cmdGroups = groups; cmdSet = set; - vshInitDebug(ctl); - if (ctl->imode && vshReadlineInit(ctl) < 0) + if (vshInitDebug(ctl) < 0 || + (ctl->imode && vshReadlineInit(ctl) < 0)) return false; return true; @@ -3033,7 +3035,8 @@ vshInitReload(vshControl *ctl) return false; } - vshInitDebug(ctl); + if (vshInitDebug(ctl) < 0) + return false; if (ctl->imode) vshReadlineDeinit(ctl); -- 2.5.5
Since commit 834c5720 which extracted the generic functionality out of virsh and made it available for other clients like virt-admin to make use of it, it also introduced a bug when it renamed the original VIRSH_ environment variables to VSH_ variables. Virt-admin of course suffers from the same bug, so this patch modifies the generic module vsh.c to construct the correct name for environment variables of each client from information it has. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1357363 Signed-off-by: Erik Skultety <eskultet@redhat.com> --- tools/virsh.c | 1 + tools/virt-admin.c | 1 + tools/vsh.c | 23 ++++++++++++----------- tools/vsh.h | 1 + 4 files changed, 15 insertions(+), 11 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 5dc482d..f74698f 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -935,6 +935,7 @@ main(int argc, char **argv) memset(ctl, 0, sizeof(vshControl)); memset(&virshCtl, 0, sizeof(virshControl)); ctl->name = "virsh"; /* hardcoded name of the binary */ + ctl->env_prefix = "VIRSH"; ctl->log_fd = -1; /* Initialize log file descriptor */ ctl->debug = VSH_DEBUG_DEFAULT; ctl->hooks = &hooks; diff --git a/tools/virt-admin.c b/tools/virt-admin.c index a59c4c7..2ae05da 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1340,6 +1340,7 @@ main(int argc, char **argv) memset(ctl, 0, sizeof(vshControl)); memset(&virtAdminCtl, 0, sizeof(vshAdmControl)); ctl->name = "virt-admin"; /* hardcoded name of the binary */ + ctl->env_prefix = "VIRT_ADMIN"; ctl->log_fd = -1; /* Initialize log file descriptor */ ctl->debug = VSH_DEBUG_DEFAULT; ctl->hooks = &hooks; diff --git a/tools/vsh.c b/tools/vsh.c index 65bd9a4..6850c59 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2833,16 +2833,10 @@ static int vshReadlineInit(vshControl *ctl) { char *userdir = NULL; - char *name_capitalized = NULL; int max_history = 500; int ret = -1; char *histsize_env = NULL; const char *histsize_str = NULL; - const char *strings[] = { - name_capitalized, - "HISTSIZE", - NULL - }; /* Allow conditional parsing of the ~/.inputrc file. * Work around ancient readline 4.1 (hello Mac OS X), @@ -2855,8 +2849,7 @@ vshReadlineInit(vshControl *ctl) rl_basic_word_break_characters = " \t\n\\`@$><=;|&{("; - if (virStringToUpper(&name_capitalized, ctl->name) < 0 || - !(histsize_env = virStringJoin(strings, "_"))) + if (virAsprintf(&histsize_env, "%s_HISTSIZE", ctl->env_prefix) < 0) goto cleanup; /* Limit the total size of the history buffer */ @@ -2898,7 +2891,6 @@ vshReadlineInit(vshControl *ctl) cleanup: VIR_FREE(userdir); - VIR_FREE(name_capitalized); VIR_FREE(histsize_env); return ret; } @@ -2970,10 +2962,14 @@ static int vshInitDebug(vshControl *ctl) { const char *debugEnv; + char *env = NULL; if (ctl->debug == VSH_DEBUG_DEFAULT) { + if (virAsprintf(&env, "%s_DEBUG", ctl->env_prefix) < 0) + return -1; + /* log level not set from commandline, check env variable */ - debugEnv = virGetEnvAllowSUID("VSH_DEBUG"); + debugEnv = virGetEnvAllowSUID(env); if (debugEnv) { int debug; if (virStrToLong_i(debugEnv, NULL, 10, &debug) < 0 || @@ -2984,15 +2980,20 @@ vshInitDebug(vshControl *ctl) ctl->debug = debug; } } + VIR_FREE(env); } if (ctl->logfile == NULL) { + if (virAsprintf(&env, "%s_LOG_FILE", ctl->env_prefix) < 0) + return -1; + /* log file not set from cmdline */ - debugEnv = virGetEnvBlockSUID("VSH_LOG_FILE"); + debugEnv = virGetEnvBlockSUID(env); if (debugEnv && *debugEnv) { ctl->logfile = vshStrdup(ctl, debugEnv); vshOpenLogFile(ctl); } + VIR_FREE(env); } return 0; diff --git a/tools/vsh.h b/tools/vsh.h index 8d67397..7f43055 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -197,6 +197,7 @@ struct _vshControl { const char *name; /* hardcoded name of the binary that cannot * be changed without recompilation compared * to program name */ + const char *env_prefix; /* hardcoded environment variable prefix */ char *connname; /* connection name */ char *progname; /* program name */ vshCmd *cmd; /* the current command */ -- 2.5.5
On Thu, Jul 28, 2016 at 01:18:03PM +0200, Erik Skultety wrote:
According to https://bugzilla.redhat.com/show_bug.cgi?id=1357363, most of virt-admin's environment variables are not taken into effect when set. As it turned out, virsh suffers from the very same problem which resides in the generic vsh.c module.
Erik Skultety (2): vsh: Make vshInitDebug return int instead of void tools: Make use of the correct environment variables
ACK series && SFF (safe for freeze)
tools/virsh.c | 1 + tools/virt-admin.c | 1 + tools/vsh.c | 34 +++++++++++++++++++--------------- tools/vsh.h | 1 + 4 files changed, 22 insertions(+), 15 deletions(-)
-- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 28/07/16 13:49, Martin Kletzander wrote:
On Thu, Jul 28, 2016 at 01:18:03PM +0200, Erik Skultety wrote:
According to https://bugzilla.redhat.com/show_bug.cgi?id=1357363, most of virt-admin's environment variables are not taken into effect when set. As it turned out, virsh suffers from the very same problem which resides in the generic vsh.c module.
Erik Skultety (2): vsh: Make vshInitDebug return int instead of void tools: Make use of the correct environment variables
ACK series && SFF (safe for freeze)
tools/virsh.c | 1 + tools/virt-admin.c | 1 + tools/vsh.c | 34 +++++++++++++++++++--------------- tools/vsh.h | 1 + 4 files changed, 22 insertions(+), 15 deletions(-)
-- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Pushed now. Thanks for review. Erik
participants (2)
-
Erik Skultety -
Martin Kletzander