[libvirt] [PATCH v2]virsh: track alias option and improve error message when option duplicates its alias
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> commit 2b172a8effa712aee97a21a64d2d02060958f9b2 allow alias to expand to opt=value pair. That means alias may not look alike since then. With this patch we will also track alias. If we type command with one option and another marked as its alias, we will get an error message like: error: option '--AA' duplicates its alias '--AAA' Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- v2: do not depend on orders of options in array. Alias could be anywhere except after the default one. tools/virsh.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tools/virsh.c b/tools/virsh.c index bad78c9..ffead84 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1071,6 +1071,7 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, size_t i; const vshCmdOptDef *ret = NULL; char *alias = NULL; + char *tmp_name = NULL; if (STREQ(name, helpopt.name)) { return &helpopt; @@ -1101,6 +1102,13 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, if (VIR_STRDUP(*optstr, value + 1) < 0) goto cleanup; } + tmp_name = vshMalloc(NULL, strlen(name) + 3); + snprintf(tmp_name, strlen(name) + 3, "--%s", name); + if (strstr(rl_line_buffer, tmp_name)) { + vshError(ctl, _("option '--%s' duplicates its alias '--%s'"), + cmd->opts[i].name, name); + goto cleanup; + } continue; } if ((*opts_seen & (1 << i)) && opt->type != VSH_OT_ARGV) { @@ -1119,6 +1127,7 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, cmd->name, name); } cleanup: + VIR_FREE(tmp_name); VIR_FREE(alias); return ret; } -- 1.8.2.1
-----Original Message----- From: Chen Hanxiao [mailto:chenhanxiao@cn.fujitsu.com] Sent: Thursday, October 31, 2013 8:19 AM To: libvir-list@redhat.com Cc: chenhanxiao@cn.fujitsu.com Subject: [libvirt][PATCH v2]virsh: track alias option and improve error message when option duplicates its alias
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
commit 2b172a8effa712aee97a21a64d2d02060958f9b2 allow alias to expand to opt=value pair. That means alias may not look alike since then.
With this patch we will also track alias. If we type command with one option and another marked as its alias, we will get an error message like: error: option '--AA' duplicates its alias '--AAA'
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- v2: do not depend on orders of options in array. Alias could be anywhere except after the default one.
tools/virsh.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/tools/virsh.c b/tools/virsh.c index bad78c9..ffead84 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1071,6 +1071,7 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, size_t i; const vshCmdOptDef *ret = NULL; char *alias = NULL; + char *tmp_name = NULL;
if (STREQ(name, helpopt.name)) { return &helpopt; @@ -1101,6 +1102,13 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, if (VIR_STRDUP(*optstr, value + 1) < 0) goto cleanup; } + tmp_name = vshMalloc(NULL, strlen(name) + 3); + snprintf(tmp_name, strlen(name) + 3, "--%s", name); + if (strstr(rl_line_buffer, tmp_name)) { + vshError(ctl, _("option '--%s' duplicates its alias '--%s'"), + cmd->opts[i].name, name); + goto cleanup; + } continue; } if ((*opts_seen & (1 << i)) && opt->type != VSH_OT_ARGV) { @@ -1119,6 +1127,7 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, cmd->name, name); } cleanup: + VIR_FREE(tmp_name); VIR_FREE(alias); return ret; } -- 1.8.2.1
ping
-----Original Message----- From: libvir-list-bounces@redhat.com [mailto:libvir-list-bounces@redhat.com] On Behalf Of Chen Hanxiao Sent: Friday, November 08, 2013 4:31 PM To: libvir-list@redhat.com Subject: Re: [libvirt] [PATCH v2]virsh: track alias option and improve error message when option duplicates its alias
-----Original Message----- From: Chen Hanxiao [mailto:chenhanxiao@cn.fujitsu.com] Sent: Thursday, October 31, 2013 8:19 AM To: libvir-list@redhat.com Cc: chenhanxiao@cn.fujitsu.com Subject: [libvirt][PATCH v2]virsh: track alias option and improve error message when option duplicates its alias
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
commit 2b172a8effa712aee97a21a64d2d02060958f9b2 allow alias to expand to opt=value pair. That means alias may not look alike since then. -- 1.8.2.1
ping
ping
On 10/31/13 01:18, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
commit 2b172a8effa712aee97a21a64d2d02060958f9b2 allow alias to expand to opt=value pair. That means alias may not look alike since then.
With this patch we will also track alias. If we type command with one option and another marked as its alias, we will get an error message like: error: option '--AA' duplicates its alias '--AAA'
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- v2: do not depend on orders of options in array. Alias could be anywhere except after the default one.
tools/virsh.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/tools/virsh.c b/tools/virsh.c index bad78c9..ffead84 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1071,6 +1071,7 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, size_t i; const vshCmdOptDef *ret = NULL; char *alias = NULL; + char *tmp_name = NULL;
if (STREQ(name, helpopt.name)) { return &helpopt; @@ -1101,6 +1102,13 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, if (VIR_STRDUP(*optstr, value + 1) < 0) goto cleanup; } + tmp_name = vshMalloc(NULL, strlen(name) + 3);
"ctl" is passed to this function so you can pass it along to vshMalloc
+ snprintf(tmp_name, strlen(name) + 3, "--%s", name);
and you can completely avoid these two lines above by using virAsprintf. Also this whole code is in a loop and you are allocating the buffer at EVERY iteration and not freeing it afterwards. This would leak tons of memory.
+ if (strstr(rl_line_buffer, tmp_name)) {
rl_line_buffer can possibly be NULL here which makes virsh crash here. (see test failure below)
+ vshError(ctl, _("option '--%s' duplicates its alias '--%s'"), + cmd->opts[i].name, name); + goto cleanup; + } continue; } if ((*opts_seen & (1 << i)) && opt->type != VSH_OT_ARGV) { @@ -1119,6 +1127,7 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, cmd->name, name); } cleanup: + VIR_FREE(tmp_name); VIR_FREE(alias); return ret; }
This patch also makes the testsuite fail with: ~/libvirt/tests $ VIR_TEST_DEBUG=2 VIR_TEST_RANGE=50-55 ./virshtest TEST: virshtest 50) virsh echo 33 ... libvirt: error : internal error: Child process (3746826) unexpected fatal signal 11 FAILED 51) virsh echo 34 ... libvirt: error : internal error: Child process (3746843) unexpected fatal signal 11 FAILED 52) virsh echo 35 ... libvirt: error : internal error: Child process (3746857) unexpected fatal signal 11 FAILED Please run the testsuite before posting patches. You are wasting review time otherwise. Peter P.S: gdb session tracing the problem: (gdb) run echo --str hellp Starting program: /home/pipo/libvirt/tools/.libs/virsh echo --str hellp warning: Could not load shared library symbols for linux-vdso.so.1. Do you need "set solib-search-path" or "set sysroot"? [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". Program received signal SIGSEGV, Segmentation fault. 0x00007ffff33bfdf5 in __strstr_sse42 () from /lib64/libc.so.6 (gdb) t a a bt Thread 1 (Thread 0x7ffff7faf840 (LWP 3746903)): #0 0x00007ffff33bfdf5 in __strstr_sse42 () from /lib64/libc.so.6 #1 0x0000555555576577 in vshCmddefGetOption (ctl=0x7fffffffd980, cmd=0x5555557c9230 <virshCmds+80>, name=0x55555581c460 "string", opts_seen=0x7fffffffd6bc, opt_index=0x7fffffffd6c0, optstr=0x7fffffffd6d0) at virsh.c:1107 #2 0x0000555555578295 in vshCommandParse (ctl=0x7fffffffd980, parser=0x7fffffffd760) at virsh.c:1897 #3 0x0000555555578944 in vshCommandArgvParse (ctl=0x7fffffffd980, nargs=3, argv=0x7fffffffdb60) at virsh.c:2056 #4 0x000055555557b91b in vshParseArgv (ctl=0x7fffffffd980, argc=4, argv=0x7fffffffdb58) at virsh.c:3236 #5 0x000055555557bbfb in main (argc=4, argv=0x7fffffffdb58) at virsh.c:3361 (gdb) up #1 0x0000555555576577 in vshCmddefGetOption (ctl=0x7fffffffd980, cmd=0x5555557c9230 <virshCmds+80>, name=0x55555581c460 "string", opts_seen=0x7fffffffd6bc, opt_index=0x7fffffffd6c0, optstr=0x7fffffffd6d0) at virsh.c:1107 1107 if (strstr(rl_line_buffer, tmp_name)) { (gdb) p rl_line_buffer $1 = 0x0
-----Original Message----- From: Peter Krempa [mailto:pkrempa@redhat.com] Sent: Wednesday, November 13, 2013 5:49 PM To: Chen Hanxiao; libvir-list@redhat.com Subject: Re: [libvirt] [PATCH v2]virsh: track alias option and improve error message when option duplicates its alias
On 10/31/13 01:18, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
commit 2b172a8effa712aee97a21a64d2d02060958f9b2 allow alias to expand to opt=value pair. That means alias may not look alike since then.
With this patch we will also track alias. If we type command with one option and another marked as its alias, we will get an error message like: error: option '--AA' duplicates its alias '--AAA'
Signed-off-by: Chen Hanxiao <chenhanxiao@cn.fujitsu.com> --- v2: do not depend on orders of options in array. Alias could be anywhere except after the default one.
tools/virsh.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/tools/virsh.c b/tools/virsh.c index bad78c9..ffead84 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1071,6 +1071,7 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, size_t i; const vshCmdOptDef *ret = NULL; char *alias = NULL; + char *tmp_name = NULL;
if (STREQ(name, helpopt.name)) { return &helpopt; @@ -1101,6 +1102,13 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, if (VIR_STRDUP(*optstr, value + 1) < 0) goto cleanup; } + tmp_name = vshMalloc(NULL, strlen(name) + 3);
"ctl" is passed to this function so you can pass it along to vshMalloc
+ snprintf(tmp_name, strlen(name) + 3, "--%s", name);
and you can completely avoid these two lines above by using virAsprintf.
Also this whole code is in a loop and you are allocating the buffer at EVERY iteration and not freeing it afterwards. This would leak tons of memory.
+ if (strstr(rl_line_buffer, tmp_name)) {
rl_line_buffer can possibly be NULL here which makes virsh crash here. (see test failure below)
+ vshError(ctl, _("option '--%s' duplicates its alias '--%s'"), + cmd->opts[i].name, name); + goto cleanup; + } continue; } if ((*opts_seen & (1 << i)) && opt->type != VSH_OT_ARGV) { @@ -1119,6 +1127,7 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, cmd->name, name); } cleanup: + VIR_FREE(tmp_name); VIR_FREE(alias); return ret; }
This patch also makes the testsuite fail with:
~/libvirt/tests $ VIR_TEST_DEBUG=2 VIR_TEST_RANGE=50-55 ./virshtest TEST: virshtest 50) virsh echo 33 ... libvirt: error : internal error: Child process (3746826) unexpected fatal signal 11 FAILED 51) virsh echo 34 ... libvirt: error : internal error: Child process (3746843) unexpected fatal signal 11 FAILED 52) virsh echo 35 ... libvirt: error : internal error: Child process (3746857) unexpected fatal signal 11 FAILED
Please run the testsuite before posting patches. You are wasting review time otherwise.
Peter
P.S: gdb session tracing the problem:
(gdb) run echo --str hellp Starting program: /home/pipo/libvirt/tools/.libs/virsh echo --str hellp warning: Could not load shared library symbols for linux-vdso.so.1. Do you need "set solib-search-path" or "set sysroot"? [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1".
Program received signal SIGSEGV, Segmentation fault. 0x00007ffff33bfdf5 in __strstr_sse42 () from /lib64/libc.so.6 (gdb) t a a bt
Thread 1 (Thread 0x7ffff7faf840 (LWP 3746903)): #0 0x00007ffff33bfdf5 in __strstr_sse42 () from /lib64/libc.so.6 #1 0x0000555555576577 in vshCmddefGetOption (ctl=0x7fffffffd980, cmd=0x5555557c9230 <virshCmds+80>, name=0x55555581c460 "string", opts_seen=0x7fffffffd6bc, opt_index=0x7fffffffd6c0, optstr=0x7fffffffd6d0) at virsh.c:1107 #2 0x0000555555578295 in vshCommandParse (ctl=0x7fffffffd980, parser=0x7fffffffd760) at virsh.c:1897 #3 0x0000555555578944 in vshCommandArgvParse (ctl=0x7fffffffd980, nargs=3, argv=0x7fffffffdb60) at virsh.c:2056 #4 0x000055555557b91b in vshParseArgv (ctl=0x7fffffffd980, argc=4, argv=0x7fffffffdb58) at virsh.c:3236 #5 0x000055555557bbfb in main (argc=4, argv=0x7fffffffdb58) at virsh.c:3361 (gdb) up #1 0x0000555555576577 in vshCmddefGetOption (ctl=0x7fffffffd980, cmd=0x5555557c9230 <virshCmds+80>, name=0x55555581c460 "string", opts_seen=0x7fffffffd6bc, opt_index=0x7fffffffd6c0, optstr=0x7fffffffd6d0) at virsh.c:1107 1107 if (strstr(rl_line_buffer, tmp_name)) { (gdb) p rl_line_buffer $1 = 0x0
I'll be more careful next time. Sorry for my mistake and thanks for the review.
On 10/30/2013 06:18 PM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao@cn.fujitsu.com>
commit 2b172a8effa712aee97a21a64d2d02060958f9b2 allow alias to expand to opt=value pair. That means alias may not look alike since then.
With this patch we will also track alias. If we type command with one option and another marked as its alias, we will get an error message like: error: option '--AA' duplicates its alias '--AAA'
In addition to Peter's review, it helps if you make your commit message smarter by showing before-and-after scenarios, to make it obvious how the error message improved. My quick testing of the 4 possible 'before' scenarios: $ args="-c test:///default attach-disk test /dev/null vda" $ tools/virsh $args --shareable --shareable error: option --mode already seen $ tools/virsh $args --shareable --mode=shareable error: option --mode already seen $ tools/virsh $args --mode=shareable --mode=shareable error: option --mode already seen $ tools/virsh $args --mode=shareable --shareable error: option --mode already seen The existing message of "--mode already seen" is absolutely confusing for scenario 1, correct for scenario 3, and sort of confusing for scenarios 2 and 4. Your proposed message of "'--shareable' duplicates its alias '--mode'" would be wrong for scenarios 1 and 3, but works for scenarios 2 and 4. Are you improving the message for one scenario at the expense of a regression in quality of other scenarios? Furthermore, since aliases are (mostly) undocumented, the ONLY people that should be using them are historical scripts that already had a working command line, and not new users trying to experiment with the various options; and working command lines don't have a problem with duplicate options. I think you're trying to address a non-issue. Since neither the '--help' text nor tab completion should be offering an alias as a preferred spelling in the first place, it's unlikely that a human user is going to be attempting a collision between its alias and the canonical spelling. I'm inclined to just say leave good enough alone, without trying to make any further "improvements", as we have reached the point of diminishing returns. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
-----Original Message----- From: Eric Blake [mailto:eblake@redhat.com] Sent: Wednesday, November 13, 2013 9:14 PM To: Chen Hanxiao; libvir-list@redhat.com Subject: Re: [libvirt] [PATCH v2]virsh: track alias option and improve error message when option duplicates its alias
In addition to Peter's review, it helps if you make your commit message smarter by showing before-and-after scenarios, to make it obvious how the error message improved. My quick testing of the 4 possible 'before' scenarios:
$ args="-c test:///default attach-disk test /dev/null vda" $ tools/virsh $args --shareable --shareable error: option --mode already seen $ tools/virsh $args --shareable --mode=shareable error: option --mode already seen $ tools/virsh $args --mode=shareable --mode=shareable error: option --mode already seen $ tools/virsh $args --mode=shareable --shareable error: option --mode already seen
Thanks for your advice.
The existing message of "--mode already seen" is absolutely confusing for scenario 1, correct for scenario 3, and sort of confusing for scenarios 2 and 4. Your proposed message of "'--shareable' duplicates its alias '--mode'" would be wrong for scenarios 1 and 3, but works for scenarios 2 and 4. Are you improving the message for one scenario at the expense of a regression in quality of other scenarios?
I lacked of test cases about two same parameters.
Furthermore, since aliases are (mostly) undocumented, the ONLY people that should be using them are historical scripts that already had a working command line, and not new users trying to experiment with the various options; and working command lines don't have a problem with duplicate options. I think you're trying to address a non-issue. Since neither the '--help' text nor tab completion should be offering an alias as a preferred spelling in the first place, it's unlikely that a human user is going to be attempting a collision between its alias and the canonical spelling.
Thanks, I see. If we can get rid of auto completion of alias, I think it will be fine enough.
I'm inclined to just say leave good enough alone, without trying to make any further "improvements", as we have reached the point of diminishing returns.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Chen Hanxiao -
Eric Blake -
Peter Krempa