-----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.