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

[Peter Krempa]

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



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131113/9924040b/attachment-0001.sig>