[libvirt] [PATCH v2]virsh: track alias option and improve error message when option duplicates its alias
7:18 p.m.
From: Chen Hanxiao <chenhanxiao(a)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(a)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
2:31 a.m.
-----Original Message-----
From: Chen Hanxiao [mailto:chenhanxiao@cn.fujitsu.com]
Sent: Thursday, October 31, 2013 8:19 AM
To: libvir-list(a)redhat.com
Cc: chenhanxiao(a)cn.fujitsu.com
Subject: [libvirt][PATCH v2]virsh: track alias option and improve error
message
when option duplicates its alias
From: Chen Hanxiao <chenhanxiao(a)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(a)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
7:02 p.m.
New subject: [libvirt] [PATCH v2]virsh: track alias option and improve error message when option duplicates its alias
-----Original Message-----
From: libvir-list-bounces(a)redhat.com
[mailto:libvir-list-bounces@redhat.com]
On Behalf Of Chen Hanxiao
Sent: Friday, November 08, 2013 4:31 PM
To: libvir-list(a)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(a)redhat.com
> Cc: chenhanxiao(a)cn.fujitsu.com
> Subject: [libvirt][PATCH v2]virsh: track alias option and improve error
message
> when option duplicates its alias
>
> From: Chen Hanxiao <chenhanxiao(a)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
3:49 a.m.
On 10/31/13 01:18, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)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(a)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
7:54 p.m.
-----Original Message-----
From: Peter Krempa [mailto:pkrempa@redhat.com]
Sent: Wednesday, November 13, 2013 5:49 PM
To: Chen Hanxiao; libvir-list(a)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(a)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(a)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.
7:14 a.m.
On 10/30/2013 06:18 PM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)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
8:23 p.m.
-----Original Message-----
From: Eric Blake [mailto:eblake@redhat.com]
Sent: Wednesday, November 13, 2013 9:14 PM
To: Chen Hanxiao; libvir-list(a)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
4056
days inactive
4070
days old
6 comments
3 participants
participants (3)
-
Chen Hanxiao -
Eric Blake -
Peter Krempa