> -----Original Message----- > From: Peter Krempa [mailto:pkrempa@xxxxxxxxxx] > Sent: Wednesday, November 13, 2013 5:49 PM > To: Chen Hanxiao; libvir-list@xxxxxxxxxx > Subject: Re: [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@xxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxx> > > --- > > 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. > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list