[libvirt] [PATCH v2]virsh: track alias option and improve error message when option duplicates its alias

[Chen Hanxiao]

> -----Original Message-----
> From: Peter Krempa [mailto:pkrempa at redhat.com]
> Sent: Wednesday, November 13, 2013 5:49 PM
> To: Chen Hanxiao; libvir-list at 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 at 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 at 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.

>