[libvirt] [PATCH 0/4] Several virsh initialization adjustments
When looking at commit 4fdd873f, I've come to notice, that after my changes to virsh (834c5720), vshInit always calls vshReadlineInit and that is because client mode defaults to interactive which might be changed after command line arguments are parsed. So this series addresses this minor issue and provides some small tweaks and adjustments. Erik Skultety (4): virsh: Do not make interactive mode default vsh: adjust vshInit signature and remove redundant error label vsh: Introduce vshInitReload vsh: Make vshInitDebug static tools/virsh.c | 12 +++++++----- tools/vsh.c | 32 ++++++++++++++++++++++---------- tools/vsh.h | 4 ++-- 3 files changed, 31 insertions(+), 17 deletions(-) -- 2.4.3
Currently, we set interactive mode as default possibly reverting the setting after we parse the command line arguments. There's nothing particulary wrong with that, but a call to vshReadlineInit is performed always in the global initializer just because the default mode is interactive. Rather than moving vshReadlineInit call somewhere else (because another client might want to implement interactive mode only), we could make the decision if we're about to run in interactive mode once the command line is parsed. --- tools/virsh.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 391c155..a71cd47 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -786,7 +786,9 @@ virshParseArgv(vshControl *ctl, int argc, char **argv) longindex = -1; } - if (argc > optind) { + if (argc == optind) { + ctl->imode = true; + } else { /* parse command */ ctl->imode = false; if (argc - optind == 1) { @@ -846,7 +848,6 @@ main(int argc, char **argv) memset(ctl, 0, sizeof(vshControl)); memset(&virshCtl, 0, sizeof(virshControl)); ctl->name = "virsh"; /* hardcoded name of the binary */ - ctl->imode = true; /* default is interactive mode */ ctl->log_fd = -1; /* Initialize log file descriptor */ ctl->debug = VSH_DEBUG_DEFAULT; ctl->hooks = &hooks; -- 2.4.3
On Fri, Sep 04, 2015 at 01:10:03PM +0200, Erik Skultety wrote:
Currently, we set interactive mode as default possibly reverting the setting after we parse the command line arguments. There's nothing particulary wrong with that, but a call to vshReadlineInit is performed always in the global initializer just because the default mode is interactive. Rather than moving vshReadlineInit call somewhere else (because another client might want to implement interactive mode only), we could make the decision if we're about to run in interactive mode once the command line is parsed. --- tools/virsh.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Moving this patch after the introduction of vshInitReload would make sure that vshReadlineInit is called correctly in every intermediate commit. Jan
As part of the effort to stay consistent, change the vshInit signature from returning int to returning bool. Moreover, remove the unnecessary error label as there is no cleanup that would make use of it. --- tools/virsh.c | 2 +- tools/vsh.c | 14 +++++--------- tools/vsh.h | 2 +- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index a71cd47..5317be8 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -905,7 +905,7 @@ main(int argc, char **argv) if ((defaultConn = virGetEnvBlockSUID("VIRSH_DEFAULT_CONNECT_URI"))) ctl->connname = vshStrdup(ctl, defaultConn); - if (vshInit(ctl, cmdGroups, NULL) < 0) + if (!vshInit(ctl, cmdGroups, NULL)) exit(EXIT_FAILURE); if (!virshParseArgv(ctl, argc, argv) || diff --git a/tools/vsh.c b/tools/vsh.c index 54c4614..e6ecc03 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2714,20 +2714,18 @@ vshInitDebug(vshControl *ctl) /* * Initialize global data */ -int +bool vshInit(vshControl *ctl, const vshCmdGrp *groups, const vshCmdDef *set) { - int ret = -1; - if (!ctl->hooks) { vshError(ctl, "%s", _("client hooks cannot be NULL")); - goto error; + return false; } if (!groups && !set) { vshError(ctl, "%s", _("command groups and command set " "cannot both be NULL")); - goto error; + return false; } cmdGroups = groups; @@ -2735,11 +2733,9 @@ vshInit(vshControl *ctl, const vshCmdGrp *groups, const vshCmdDef *set) vshInitDebug(ctl); if (ctl->imode && vshReadlineInit(ctl) < 0) - goto error; + return false; - ret = 0; - error: - return ret; + return true; } void diff --git a/tools/vsh.h b/tools/vsh.h index 37416c7..e2e33ba 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -302,7 +302,7 @@ int vshCommandOptTimeoutToMs(vshControl *ctl, const vshCmd *cmd, int *timeout); void vshPrintExtra(vshControl *ctl, const char *format, ...) ATTRIBUTE_FMT_PRINTF(2, 3); -int vshInit(vshControl *ctl, const vshCmdGrp *groups, const vshCmdDef *set); +bool vshInit(vshControl *ctl, const vshCmdGrp *groups, const vshCmdDef *set); void vshDeinit(vshControl *ctl); void vshInitDebug(vshControl *ctl); void vshDebug(vshControl *ctl, int level, const char *format, ...) -- 2.4.3
Commit a0b6a36f separated vshInitDebug from the original vshInit (before virsh got split and vshInit became virshInit - commit 834c5720) in order to be able to debug command line parsing. After the parsing is finished, debugging is reinitialized to work properly. There might as well be other features that require re-initialization as the command line could specify parameters that override our defaults which had been set prior to calling vshArgvParse. --- tools/virsh.c | 5 +++-- tools/vsh.c | 16 ++++++++++++++++ tools/vsh.h | 1 + 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 5317be8..bb12dec 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -347,8 +347,9 @@ virshInit(vshControl *ctl) virshControlPtr priv = ctl->privData; /* Since we have the commandline arguments parsed, we need to - * re-initialize all the debugging to make it work properly */ - vshInitDebug(ctl); + * reload our initial settings to make debugging and readline + * work properly */ + vshInitReload(ctl); if (priv->conn) return false; diff --git a/tools/vsh.c b/tools/vsh.c index e6ecc03..d4059fc 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2738,6 +2738,22 @@ vshInit(vshControl *ctl, const vshCmdGrp *groups, const vshCmdDef *set) return true; } +bool +vshInitReload(vshControl *ctl) +{ + if (!cmdGroups && !cmdSet) { + vshError(ctl, "%s", _("command groups and command are both NULL " + "run vshInit before reloading")); + return false; + } + + vshInitDebug(ctl); + if (ctl->imode && vshReadlineInit(ctl) < 0) + return false; + + return true; +} + void vshDeinit(vshControl *ctl) { diff --git a/tools/vsh.h b/tools/vsh.h index e2e33ba..b687604 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -303,6 +303,7 @@ int vshCommandOptTimeoutToMs(vshControl *ctl, const vshCmd *cmd, int *timeout); void vshPrintExtra(vshControl *ctl, const char *format, ...) ATTRIBUTE_FMT_PRINTF(2, 3); bool vshInit(vshControl *ctl, const vshCmdGrp *groups, const vshCmdDef *set); +bool vshInitReload(vshControl *ctl); void vshDeinit(vshControl *ctl); void vshInitDebug(vshControl *ctl); void vshDebug(vshControl *ctl, int level, const char *format, ...) -- 2.4.3
On Fri, Sep 04, 2015 at 01:10:05PM +0200, Erik Skultety wrote:
Commit a0b6a36f separated vshInitDebug from the original vshInit (before virsh got split and vshInit became virshInit - commit 834c5720) in order to be able to debug command line parsing. After the parsing is finished, debugging is reinitialized to work properly. There might as well be other features that require re-initialization as the command line could specify parameters that override our defaults which had been set prior to calling vshArgvParse. --- tools/virsh.c | 5 +++-- tools/vsh.c | 16 ++++++++++++++++ tools/vsh.h | 1 + 3 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 5317be8..bb12dec 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -347,8 +347,9 @@ virshInit(vshControl *ctl) virshControlPtr priv = ctl->privData;
/* Since we have the commandline arguments parsed, we need to - * re-initialize all the debugging to make it work properly */ - vshInitDebug(ctl); + * reload our initial settings to make debugging and readline + * work properly */ + vshInitReload(ctl);
if (priv->conn) return false; diff --git a/tools/vsh.c b/tools/vsh.c index e6ecc03..d4059fc 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2738,6 +2738,22 @@ vshInit(vshControl *ctl, const vshCmdGrp *groups, const vshCmdDef *set) return true; }
+bool +vshInitReload(vshControl *ctl) +{ + if (!cmdGroups && !cmdSet) { + vshError(ctl, "%s", _("command groups and command are both NULL " + "run vshInit before reloading")); + return false; + } + + vshInitDebug(ctl); + if (ctl->imode && vshReadlineInit(ctl) < 0)
You add another vshReadlineInit call here, but do not remove the one in vshInit. Just calling vshInit might make sense for callers that know upfront they are running in interactive mode. But we should call vshReadlineDeInit to prevent a memory leak if they call vshInitReload too. Jan
There's no reason why debug initialization could not be made completely hidden, just like readline initialization is. The point of the global initializer vshInit is to make initialization of smaller features transparent to the user/caller. --- tools/vsh.c | 2 +- tools/vsh.h | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index d4059fc..1564d84 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2680,7 +2680,7 @@ vshReadline(vshControl *ctl, const char *prompt) /* * Initialize debug settings. */ -void +static void vshInitDebug(vshControl *ctl) { const char *debugEnv; diff --git a/tools/vsh.h b/tools/vsh.h index b687604..d4e9710 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -305,7 +305,6 @@ void vshPrintExtra(vshControl *ctl, const char *format, ...) bool vshInit(vshControl *ctl, const vshCmdGrp *groups, const vshCmdDef *set); bool vshInitReload(vshControl *ctl); void vshDeinit(vshControl *ctl); -void vshInitDebug(vshControl *ctl); void vshDebug(vshControl *ctl, int level, const char *format, ...) ATTRIBUTE_FMT_PRINTF(3, 4); -- 2.4.3
On Fri, Sep 04, 2015 at 01:10:02PM +0200, Erik Skultety wrote:
When looking at commit 4fdd873f, I've come to notice, that after my changes to virsh (834c5720), vshInit always calls vshReadlineInit and that is because client mode defaults to interactive which might be changed after command line arguments are parsed. So this series addresses this minor issue and provides some small tweaks and adjustments.
Erik Skultety (4): virsh: Do not make interactive mode default vsh: adjust vshInit signature and remove redundant error label vsh: Introduce vshInitReload vsh: Make vshInitDebug static
tools/virsh.c | 12 +++++++----- tools/vsh.c | 32 ++++++++++++++++++++++---------- tools/vsh.h | 4 ++-- 3 files changed, 31 insertions(+), 17 deletions(-)
ACK series Jan
On 04/09/15 13:47, Ján Tomko wrote:
On Fri, Sep 04, 2015 at 01:10:02PM +0200, Erik Skultety wrote:
When looking at commit 4fdd873f, I've come to notice, that after my changes to virsh (834c5720), vshInit always calls vshReadlineInit and that is because client mode defaults to interactive which might be changed after command line arguments are parsed. So this series addresses this minor issue and provides some small tweaks and adjustments.
Erik Skultety (4): virsh: Do not make interactive mode default vsh: adjust vshInit signature and remove redundant error label vsh: Introduce vshInitReload vsh: Make vshInitDebug static
tools/virsh.c | 12 +++++++----- tools/vsh.c | 32 ++++++++++++++++++++++---------- tools/vsh.h | 4 ++-- 3 files changed, 31 insertions(+), 17 deletions(-)
ACK series
Jan
Thanks for review, I fixed 3/4, moved 1/4 after 3/4 and pushed the series. Erik
participants (2)
-
Erik Skultety -
Ján Tomko