[libvirt PATCH 0/5] vsh: use g_auto more (glib chronicles)
Except for vshCommandParse. -EPARSE Ján Tomko (5): vsh: do not cast away const vsh: cmdEcho: use separate variable for argument vsh: use g_auto where possible vsh: remove pointless cleanup labels vsh: use g_clear_pointer tools/vsh.c | 91 ++++++++++++++++++----------------------------------- 1 file changed, 31 insertions(+), 60 deletions(-) -- 2.31.1
Instead of using the same variable to store either a const pointer or an allocated string, always make a copy. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/vsh.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 123284c636..3bbaecd2ea 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -318,7 +318,7 @@ vshCmddefCheckInternals(vshControl *ctl, case VSH_OT_ALIAS: { size_t j; - char *name = (char *)opt->help; /* cast away const */ + g_autofree char *name = NULL; char *p; if (opt->flags || !opt->help) { @@ -326,15 +326,16 @@ vshCmddefCheckInternals(vshControl *ctl, opt->name, cmd->name); return -1; /* alias options are tracked by the original name */ } - if ((p = strchr(name, '='))) - name = g_strndup(name, p - name); + if ((p = strchr(opt->help, '='))) + name = g_strndup(opt->help, p - opt->help); + else + name = g_strdup(opt->help); for (j = i + 1; cmd->opts[j].name; j++) { if (STREQ(name, cmd->opts[j].name) && cmd->opts[j].type != VSH_OT_ALIAS) break; } - if (name != opt->help) { - VIR_FREE(name); + if (p) { /* If alias comes with value, replacement must not be bool */ if (cmd->opts[j].type == VSH_OT_BOOL) { vshError(ctl, _("alias '%s' of command '%s' has mismatched alias type"), -- 2.31.1
On Tue, Aug 10, 2021 at 19:14:39 +0200, Ján Tomko wrote:
Instead of using the same variable to store either a const pointer or an allocated string, always make a copy.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/vsh.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Do not use 'arg' which is later used for an allocated string. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/vsh.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 3bbaecd2ea..bf32a8dc22 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -3199,17 +3199,16 @@ cmdEcho(vshControl *ctl, const vshCmd *cmd) while ((opt = vshCommandOptArgv(ctl, cmd, opt))) { char *str; g_auto(virBuffer) xmlbuf = VIR_BUFFER_INITIALIZER; - - arg = opt->data; + const char *curr = opt->data; if (count) virBufferAddChar(&buf, ' '); if (xml) { - virBufferEscapeString(&xmlbuf, "%s", arg); + virBufferEscapeString(&xmlbuf, "%s", curr); str = virBufferContentAndReset(&xmlbuf); } else { - str = g_strdup(arg); + str = g_strdup(curr); } if (shell) -- 2.31.1
On Tue, Aug 10, 2021 at 19:14:40 +0200, Ján Tomko wrote:
Do not use 'arg' which is later used for an allocated string.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/vsh.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com> There's quite some room for rewriting the code to even avoid the use of temps and string copies though.
Excluding vshCommandParse. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/vsh.c | 46 ++++++++++++++-------------------------------- 1 file changed, 14 insertions(+), 32 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index bf32a8dc22..71fbb7401f 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -122,7 +122,7 @@ int vshStringToArray(const char *str, char ***array) { - char *str_copied = g_strdup(str); + g_autofree char *str_copied = g_strdup(str); char *str_tok = NULL; char *tmp; unsigned int nstr_tokens = 0; @@ -163,7 +163,6 @@ vshStringToArray(const char *str, arr[nstr_tokens++] = g_strdup(str_tok); *array = arr; - VIR_FREE(str_copied); return nstr_tokens; } @@ -434,7 +433,7 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, { size_t i; const vshCmdOptDef *ret = NULL; - char *alias = NULL; + g_autofree char *alias = NULL; if (STREQ(name, helpopt.name)) return &helpopt; @@ -481,7 +480,6 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, cmd->name, name); } cleanup: - VIR_FREE(alias); return ret; } @@ -1814,7 +1812,7 @@ void vshDebug(vshControl *ctl, int level, const char *format, ...) { va_list ap; - char *str; + g_autofree char *str = NULL; /* Aligning log levels to that of libvirt. * Traces with levels >= user-specified-level @@ -1831,14 +1829,13 @@ vshDebug(vshControl *ctl, int level, const char *format, ...) str = g_strdup_vprintf(format, ap); va_end(ap); fputs(str, stdout); - VIR_FREE(str); } void vshPrintExtra(vshControl *ctl, const char *format, ...) { va_list ap; - char *str; + g_autofree char *str = NULL; if (ctl && ctl->quiet) return; @@ -1847,7 +1844,6 @@ vshPrintExtra(vshControl *ctl, const char *format, ...) str = g_strdup_vprintf(format, ap); va_end(ap); fputs(str, stdout); - VIR_FREE(str); } @@ -1855,13 +1851,12 @@ void vshPrint(vshControl *ctl G_GNUC_UNUSED, const char *format, ...) { va_list ap; - char *str; + g_autofree char *str = NULL; va_start(ap, format); str = g_strdup_vprintf(format, ap); va_end(ap); fputs(str, stdout); - VIR_FREE(str); } @@ -1960,7 +1955,7 @@ void vshError(vshControl *ctl, const char *format, ...) { va_list ap; - char *str; + g_autofree char *str = NULL; if (ctl != NULL) { va_start(ap, format); @@ -1980,7 +1975,6 @@ vshError(vshControl *ctl, const char *format, ...) fprintf(stderr, "%s\n", NULLSTR(str)); fflush(stderr); - VIR_FREE(str); } @@ -2186,7 +2180,7 @@ vshOutputLogFile(vshControl *ctl, int log_level, const char *msg_format, va_list ap) { g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - char *str = NULL; + g_autofree char *str = NULL; size_t len; const char *lvl = ""; g_autoptr(GDateTime) now = g_date_time_new_now_local(); @@ -2237,13 +2231,11 @@ vshOutputLogFile(vshControl *ctl, int log_level, const char *msg_format, if (safewrite(ctl->log_fd, str, len) < 0) goto error; - VIR_FREE(str); return; error: vshCloseLogFile(ctl); vshError(ctl, "%s", _("failed to write the log file")); - VIR_FREE(str); } /** @@ -2359,7 +2351,7 @@ vshAskReedit(vshControl *ctl, char * vshEditWriteToTempFile(vshControl *ctl, const char *doc) { - char *ret; + g_autofree char *ret = NULL; const char *tmpdir; int fd; @@ -2370,7 +2362,6 @@ vshEditWriteToTempFile(vshControl *ctl, const char *doc) if (fd == -1) { vshError(ctl, _("g_mkstemp_full: failed to create temporary file: %s"), g_strerror(errno)); - VIR_FREE(ret); return NULL; } @@ -2379,14 +2370,12 @@ vshEditWriteToTempFile(vshControl *ctl, const char *doc) ret, g_strerror(errno)); VIR_FORCE_CLOSE(fd); unlink(ret); - VIR_FREE(ret); return NULL; } if (VIR_CLOSE(fd) < 0) { vshError(ctl, _("close: %s: failed to write or close temporary file: %s"), ret, g_strerror(errno)); unlink(ret); - VIR_FREE(ret); return NULL; } @@ -2406,7 +2395,7 @@ int vshEditFile(vshControl *ctl, const char *filename) { const char *editor; - virCommand *cmd; + g_autoptr(virCommand) cmd = NULL; int ret = -1; int outfd = STDOUT_FILENO; int errfd = STDERR_FILENO; @@ -2450,7 +2439,6 @@ vshEditFile(vshControl *ctl, const char *filename) ret = 0; cleanup: - virCommandFree(cmd); return ret; } @@ -2806,10 +2794,10 @@ vshReadlineCharIsQuoted(char *line, int idx) static int vshReadlineInit(vshControl *ctl) { - char *userdir = NULL; + g_autofree char *userdir = NULL; int max_history = 500; int ret = -1; - char *histsize_env = NULL; + g_autofree char *histsize_env = NULL; const char *histsize_str = NULL; const char *break_characters = " \t\n`@$><=;|&{("; const char *quote_characters = "\"'"; @@ -2856,8 +2844,6 @@ vshReadlineInit(vshControl *ctl) ret = 0; cleanup: - VIR_FREE(userdir); - VIR_FREE(histsize_env); return ret; } @@ -2940,7 +2926,7 @@ static int vshInitDebug(vshControl *ctl) { const char *debugEnv; - char *env = NULL; + g_autofree char *env = NULL; if (ctl->debug == VSH_DEBUG_DEFAULT) { env = g_strdup_printf("%s_DEBUG", ctl->env_prefix); @@ -2957,7 +2943,6 @@ vshInitDebug(vshControl *ctl) ctl->debug = debug; } } - VIR_FREE(env); } if (ctl->logfile == NULL) { @@ -2969,7 +2954,6 @@ vshInitDebug(vshControl *ctl) ctl->logfile = g_strdup(debugEnv); vshOpenLogFile(ctl); } - VIR_FREE(env); } return 0; @@ -3186,7 +3170,7 @@ cmdEcho(vshControl *ctl, const vshCmd *cmd) bool err = false; int count = 0; const vshCmdOpt *opt = NULL; - char *arg; + g_autofree char *arg = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; if (vshCommandOptBool(cmd, "shell")) @@ -3197,7 +3181,7 @@ cmdEcho(vshControl *ctl, const vshCmd *cmd) err = true; while ((opt = vshCommandOptArgv(ctl, cmd, opt))) { - char *str; + g_autofree char *str = NULL; g_auto(virBuffer) xmlbuf = VIR_BUFFER_INITIALIZER; const char *curr = opt->data; @@ -3216,7 +3200,6 @@ cmdEcho(vshControl *ctl, const vshCmd *cmd) else virBufferAdd(&buf, str, -1); count++; - VIR_FREE(str); } arg = virBufferContentAndReset(&buf); @@ -3226,7 +3209,6 @@ cmdEcho(vshControl *ctl, const vshCmd *cmd) else vshPrint(ctl, "%s", arg); } - VIR_FREE(arg); return true; } -- 2.31.1
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/vsh.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 1f384d4ea6..f9600bafba 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2915,10 +2915,9 @@ static int vshInitDebug(vshControl *ctl) { const char *debugEnv; - g_autofree char *env = NULL; if (ctl->debug == VSH_DEBUG_DEFAULT) { - env = g_strdup_printf("%s_DEBUG", ctl->env_prefix); + g_autofree char *env = g_strdup_printf("%s_DEBUG", ctl->env_prefix); /* log level not set from commandline, check env variable */ debugEnv = getenv(env); @@ -2935,7 +2934,7 @@ vshInitDebug(vshControl *ctl) } if (ctl->logfile == NULL) { - env = g_strdup_printf("%s_LOG_FILE", ctl->env_prefix); + g_autofree char *env = g_strdup_printf("%s_LOG_FILE", ctl->env_prefix); /* log file not set from cmdline */ debugEnv = getenv(env); -- 2.31.1
Remove cleanup sections that are no longer needed, as well as unnecessary 'ret' variables. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/vsh.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 71fbb7401f..f4b555fb8b 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2396,7 +2396,6 @@ vshEditFile(vshControl *ctl, const char *filename) { const char *editor; g_autoptr(virCommand) cmd = NULL; - int ret = -1; int outfd = STDOUT_FILENO; int errfd = STDERR_FILENO; @@ -2434,12 +2433,9 @@ vshEditFile(vshControl *ctl, const char *filename) if (virCommandRunAsync(cmd, NULL) < 0 || virCommandWait(cmd, NULL) < 0) { vshReportError(ctl); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - return ret; + return 0; } char * @@ -2796,7 +2792,6 @@ vshReadlineInit(vshControl *ctl) { g_autofree char *userdir = NULL; int max_history = 500; - int ret = -1; g_autofree char *histsize_env = NULL; const char *histsize_str = NULL; const char *break_characters = " \t\n`@$><=;|&{("; @@ -2821,12 +2816,12 @@ vshReadlineInit(vshControl *ctl) if ((histsize_str = getenv(histsize_env))) { if (virStrToLong_i(histsize_str, NULL, 10, &max_history) < 0) { vshError(ctl, _("Bad $%s value."), histsize_env); - goto cleanup; + return -1; } else if (max_history > HISTSIZE_MAX || max_history < 0) { vshError(ctl, _("$%s value should be between 0 " "and %d"), histsize_env, HISTSIZE_MAX); - goto cleanup; + return -1; } } stifle_history(max_history); @@ -2841,10 +2836,7 @@ vshReadlineInit(vshControl *ctl) ctl->historyfile = g_strdup_printf("%s/history", ctl->historydir); read_history(ctl->historyfile); - ret = 0; - - cleanup: - return ret; + return 0; } static void -- 2.31.1
On Tue, Aug 10, 2021 at 19:14:42 +0200, Ján Tomko wrote:
Remove cleanup sections that are no longer needed, as well as unnecessary 'ret' variables.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/vsh.c | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Replace remaining uses of VIR_FREE with g_clear_pointer. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/vsh.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index f4b555fb8b..1f384d4ea6 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2253,10 +2253,7 @@ vshCloseLogFile(vshControl *ctl) g_strerror(errno)); } - if (ctl->logfile) { - VIR_FREE(ctl->logfile); - ctl->logfile = NULL; - } + g_clear_pointer(&ctl->logfile, g_free); } #ifndef WIN32 @@ -2852,8 +2849,8 @@ vshReadlineDeinit(vshControl *ctl) } } - VIR_FREE(ctl->historydir); - VIR_FREE(ctl->historyfile); + g_clear_pointer(&ctl->historydir, g_free); + g_clear_pointer(&ctl->historyfile, g_free); } char * -- 2.31.1
On Tue, Aug 10, 2021 at 19:14:43 +0200, Ján Tomko wrote:
Replace remaining uses of VIR_FREE with g_clear_pointer.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/vsh.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Ján Tomko -
Peter Krempa