[PATCH 0/6] virsh: Some fixes noted when reviewing Jan's series
Peter Krempa (6): virsh: Un-document 'virsh echo' virshtest: Don't use both '--xml' and '--shell' for 'virsh echo' virsh: cmdEcho: Make '--xml' and '--shell' mutually exclusive virsh: cmdEcho: Rewrite with new buffer helpers virsh: Add testing for vshStringToArray vshStringToArray: Rewrite using 'g_strsplit' docs/manpages/virsh.rst | 17 ------ tests/virshtest.c | 20 +++++-- tools/vsh.c | 115 +++++++++++++++++++--------------------- 3 files changed, 69 insertions(+), 83 deletions(-) -- 2.31.1
Note that it's for internal testing use and remove the manpage entry. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 17 ----------------- tools/vsh.c | 4 ++-- 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 20936994ce..4075929a76 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -907,23 +907,6 @@ print that all CPU models are accepted for these architectures and the actual list of supported CPU models can be checked in the domain capabilities XML. -echo ----- - -**Syntax:** - -:: - - echo [--shell] [--xml] [err...] [arg...] - -Echo back each *arg*, separated by space. If *--shell* is -specified, then the output will be single-quoted where needed, so that -it is suitable for reuse in a shell context. If *--xml* is -specified, then the output will be escaped for use in XML. -If *--err* is specified, prefix ``"error: "`` and output to stderr -instead of stdout. - - hypervisor-cpu-compare ---------------------- diff --git a/tools/vsh.c b/tools/vsh.c index f9600bafba..f44db5d56d 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -3139,10 +3139,10 @@ const vshCmdOptDef opts_echo[] = { const vshCmdInfo info_echo[] = { {.name = "help", - .data = N_("echo arguments") + .data = N_("echo arguments. Used for internal testing.") }, {.name = "desc", - .data = N_("Echo back arguments, possibly with quoting.") + .data = N_("Echo back arguments, possibly with quoting. Used for internal testing.") }, {.name = NULL} }; -- 2.31.1
Escaping for both shell and XML makes no sense. Use one at time so that we can forbid use of both. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virshtest.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/virshtest.c b/tests/virshtest.c index 53db2aa19a..07c27428ae 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -432,9 +432,8 @@ mymain(void) DO_TEST(15, "a A 0 + * ; . ' " / ? = \n < > &\n", "echo", "--xml", "a", "A", "0", "+", "*", ";", ".", "'", "\"", "/", "?", "=", " ", "\n", "<", ">", "&"); - DO_TEST(16, "a A 0 + '*' ';' . ''' '"' / '?' = ' ' '\n' '<'" - " '>' '&'\n", - "echo", "--shell", "--xml", "a", "A", "0", "+", "*", ";", ".", "'", + DO_TEST(16, "a A 0 + '*' ';' . ''\\''' '\"' / '?' = ' ' '\n' '<' '>' '&'\n", + "echo", "--shell", "a", "A", "0", "+", "*", ";", ".", "\'", "\"", "/", "?", "=", " ", "\n", "<", ">", "&"); DO_TEST(17, "\n", "echo", ""); @@ -443,7 +442,7 @@ mymain(void) DO_TEST(19, "\n", "echo", "--xml", ""); DO_TEST(20, "''\n", - "echo", "--xml", "--shell", ""); + "echo", "--shell", ""); DO_TEST(21, "\n", "echo ''"); DO_TEST(22, "''\n", @@ -451,7 +450,7 @@ mymain(void) DO_TEST(23, "\n", "echo --xml ''"); DO_TEST(24, "''\n", - "echo --xml --shell \"\"''"); + "echo --shell \"\"''"); /* Tests of -- handling. */ DO_TEST(25, "a\n", -- 2.31.1
Initialize the flags earlier and use VSH_EXCLUSIVE_OPTIONS_VAR to declare the conflicting options as exclusive. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index f44db5d56d..009c93254c 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -3153,20 +3153,15 @@ const vshCmdInfo info_echo[] = { bool cmdEcho(vshControl *ctl, const vshCmd *cmd) { - bool shell = false; - bool xml = false; - bool err = false; + bool shell = vshCommandOptBool(cmd, "shell"); + bool xml = vshCommandOptBool(cmd, "xml"); + bool err = vshCommandOptBool(cmd, "err"); int count = 0; const vshCmdOpt *opt = NULL; g_autofree char *arg = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - if (vshCommandOptBool(cmd, "shell")) - shell = true; - if (vshCommandOptBool(cmd, "xml")) - xml = true; - if (vshCommandOptBool(cmd, "err")) - err = true; + VSH_EXCLUSIVE_OPTIONS_VAR(shell, xml); while ((opt = vshCommandOptArgv(ctl, cmd, opt))) { g_autofree char *str = NULL; -- 2.31.1
Remove the need for temporary strings by fillin the output buffer directly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 009c93254c..2456267426 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -3156,7 +3156,6 @@ cmdEcho(vshControl *ctl, const vshCmd *cmd) bool shell = vshCommandOptBool(cmd, "shell"); bool xml = vshCommandOptBool(cmd, "xml"); bool err = vshCommandOptBool(cmd, "err"); - int count = 0; const vshCmdOpt *opt = NULL; g_autofree char *arg = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; @@ -3164,27 +3163,21 @@ cmdEcho(vshControl *ctl, const vshCmd *cmd) VSH_EXCLUSIVE_OPTIONS_VAR(shell, xml); while ((opt = vshCommandOptArgv(ctl, cmd, opt))) { - g_autofree char *str = NULL; - g_auto(virBuffer) xmlbuf = VIR_BUFFER_INITIALIZER; const char *curr = opt->data; - if (count) - virBufferAddChar(&buf, ' '); - if (xml) { - virBufferEscapeString(&xmlbuf, "%s", curr); - str = virBufferContentAndReset(&xmlbuf); + virBufferEscapeString(&buf, "%s", curr); + } else if (shell) { + virBufferEscapeShell(&buf, curr); } else { - str = g_strdup(curr); + virBufferAdd(&buf, curr, -1); } - if (shell) - virBufferEscapeShell(&buf, str); - else - virBufferAdd(&buf, str, -1); - count++; + virBufferAddChar(&buf, ' '); } + virBufferTrim(&buf, " "); + arg = virBufferContentAndReset(&buf); if (arg) { if (err) -- 2.31.1
Add a '--split' switch for the 'virsh echo' command and add few test cases to the virshtest. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/virshtest.c | 11 +++++++++++ tools/vsh.c | 15 +++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/tests/virshtest.c b/tests/virshtest.c index 07c27428ae..751e8ffc49 100644 --- a/tests/virshtest.c +++ b/tests/virshtest.c @@ -487,6 +487,17 @@ mymain(void) DO_TEST(46, "a\n", "#unbalanced; 'quotes\"\necho a # b"); DO_TEST(47, "a\n", "\\# ignored;echo a\n'#also' ignored"); + /* test of splitting in vshStringToArray */ + DO_TEST(48, "a\nb,c,\nd,,e,,\nf,,,e\n", + "-q", "echo", "--split", "a,b,,c,,,d,,,,e,,,,,f,,,,,,e"); + DO_TEST(49, "\na\nb,c,\nd,,e,,\nf,,,e\n\n", + "-q", "echo", "--split", ",a,b,,c,,,d,,,,e,,,,,f,,,,,,e,"); + DO_TEST(50, ",a\nb,c,\nd,,e,,\nf,,,e,\n", + "-q", "echo", "--split", ",,a,b,,c,,,d,,,,e,,,,,f,,,,,,e,,"); + DO_TEST(51, ",\na\nb,c,\nd,,e,,\nf,,,e,\n\n", + "-q", "echo", "--split", ",,,a,b,,c,,,d,,,,e,,,,,f,,,,,,e,,,"); + DO_TEST(52, ",,a\nb,c,\nd,,e,,\nf,,,e,,\n", + "-q", "echo", "--split", ",,,,a,b,,c,,,d,,,,e,,,,,f,,,,,,e,,,,"); # undef DO_TEST VIR_FREE(custom_uri); diff --git a/tools/vsh.c b/tools/vsh.c index 2456267426..cdcf67684e 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -3118,6 +3118,10 @@ const vshCmdOptDef opts_echo[] = { .type = VSH_OT_BOOL, .help = N_("escape for XML use") }, + {.name = "split", + .type = VSH_OT_BOOL, + .help = N_("split each argument on ','; ',,' is an escape sequence") + }, {.name = "err", .type = VSH_OT_BOOL, .help = N_("output to stderr"), @@ -3156,11 +3160,14 @@ cmdEcho(vshControl *ctl, const vshCmd *cmd) bool shell = vshCommandOptBool(cmd, "shell"); bool xml = vshCommandOptBool(cmd, "xml"); bool err = vshCommandOptBool(cmd, "err"); + bool split = vshCommandOptBool(cmd, "split"); const vshCmdOpt *opt = NULL; g_autofree char *arg = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; VSH_EXCLUSIVE_OPTIONS_VAR(shell, xml); + VSH_EXCLUSIVE_OPTIONS_VAR(shell, split); + VSH_EXCLUSIVE_OPTIONS_VAR(xml, split); while ((opt = vshCommandOptArgv(ctl, cmd, opt))) { const char *curr = opt->data; @@ -3169,6 +3176,14 @@ cmdEcho(vshControl *ctl, const vshCmd *cmd) virBufferEscapeString(&buf, "%s", curr); } else if (shell) { virBufferEscapeShell(&buf, curr); + } else if (split) { + g_auto(GStrv) spl = NULL; + GStrv n; + + vshStringToArray(curr, &spl); + + for (n = spl; *n; n++) + virBufferAsprintf(&buf, "%s\n", *n); } else { virBufferAdd(&buf, curr, -1); } -- 2.31.1
Use 'g_strsplit' to split the strings and then concatenate back when the escape sequence (',,') is used. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/vsh.c | 62 ++++++++++++++++++++++------------------------------- 1 file changed, 26 insertions(+), 36 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index cdcf67684e..202a507f2e 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -122,48 +122,38 @@ int vshStringToArray(const char *str, char ***array) { - g_autofree char *str_copied = g_strdup(str); - char *str_tok = NULL; - char *tmp; - unsigned int nstr_tokens = 0; - char **arr = NULL; - size_t len = strlen(str_copied); - - /* tokenize the string from user and save its parts into an array */ - nstr_tokens = 1; - - /* count the delimiters, recognizing ,, as an escape for a - * literal comma */ - str_tok = str_copied; - while ((str_tok = strchr(str_tok, ','))) { - if (str_tok[1] == ',') - str_tok++; - else - nstr_tokens++; - str_tok++; - } + g_auto(GStrv) tmp = NULL; + GStrv n; + size_t ntoks = 0; + bool concat = false; + + tmp = g_strsplit(str, ",", 0); + + *array = g_new0(char *, g_strv_length(tmp) + 1); + (*array)[ntoks++] = g_strdup(tmp[0]); - /* reserve the NULL element at the end */ - arr = g_new0(char *, nstr_tokens + 1); + /* undo splitting of comma escape (',,') by concatenating back on empty strings */ + for (n = tmp + 1; n[0]; n++) { + if (concat) { + g_autofree char *old = (*array)[ntoks - 1]; - /* tokenize the input string, while treating ,, as a literal comma */ - nstr_tokens = 0; - tmp = str_tok = str_copied; - while ((tmp = strchr(tmp, ','))) { - if (tmp[1] == ',') { - memmove(&tmp[1], &tmp[2], len - (tmp - str_copied) - 2 + 1); - len--; - tmp++; + (*array)[ntoks - 1] = g_strconcat(old, ",", n[0], NULL); + concat = false; continue; } - *tmp++ = '\0'; - arr[nstr_tokens++] = g_strdup(str_tok); - str_tok = tmp; + + if (strlen(n[0]) == 0) { + concat = true; + } else { + (*array)[ntoks++] = g_strdup(n[0]); + } } - arr[nstr_tokens++] = g_strdup(str_tok); - *array = arr; - return nstr_tokens; + /* corner case of ending with a single comma */ + if (concat) + (*array)[ntoks++] = g_strdup(""); + + return ntoks; } virErrorPtr last_error; -- 2.31.1
On Thu, Aug 12, 2021 at 03:01:30PM +0200, Peter Krempa wrote:
Peter Krempa (6): virsh: Un-document 'virsh echo' virshtest: Don't use both '--xml' and '--shell' for 'virsh echo' virsh: cmdEcho: Make '--xml' and '--shell' mutually exclusive virsh: cmdEcho: Rewrite with new buffer helpers virsh: Add testing for vshStringToArray vshStringToArray: Rewrite using 'g_strsplit'
With the typo in 4/6 fixed the series is Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
docs/manpages/virsh.rst | 17 ------ tests/virshtest.c | 20 +++++-- tools/vsh.c | 115 +++++++++++++++++++--------------------- 3 files changed, 69 insertions(+), 83 deletions(-)
-- 2.31.1
On a %A in %Y, Peter Krempa wrote:
Peter Krempa (6): virsh: Un-document 'virsh echo' virshtest: Don't use both '--xml' and '--shell' for 'virsh echo' virsh: cmdEcho: Make '--xml' and '--shell' mutually exclusive virsh: cmdEcho: Rewrite with new buffer helpers virsh: Add testing for vshStringToArray vshStringToArray: Rewrite using 'g_strsplit'
docs/manpages/virsh.rst | 17 ------ tests/virshtest.c | 20 +++++-- tools/vsh.c | 115 +++++++++++++++++++--------------------- 3 files changed, 69 insertions(+), 83 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Jano Tomko -
Martin Kletzander -
Peter Krempa