[libvirt] [PATCH]virsh: track alias option and improve error message when option duplicate its alias
11:19 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' duplicate its alias '--AAA'
Signed-off-by: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
---
tools/virsh.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index bad78c9..423e2d8 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1101,11 +1101,18 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const
char *name,
if (VIR_STRDUP(*optstr, value + 1) < 0)
goto cleanup;
}
+ *opts_seen |= 1 << i;
continue;
}
if ((*opts_seen & (1 << i)) && opt->type != VSH_OT_ARGV)
{
- vshError(ctl, _("option --%s already seen"), name);
- goto cleanup;
+ if ((*opts_seen & (1 << (i - 1)))) {
+ vshError(ctl, _("option '--%s' duplicates its alias
'--%s'"),
+ cmd->opts[i].name, cmd->opts[i-1].name);
+ goto cleanup;
+ } else {
+ vshError(ctl, _("option '--%s' already seen"),
name);
+ goto cleanup;
+ }
}
*opts_seen |= 1 << i;
*opt_index = i;
--
1.8.2.1
8:41 a.m.
On 10/30/13 05:19, 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' duplicate its alias '--AAA'
s/duplicate/duplicates/ ... to match the error down below.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)cn.fujitsu.com>
---
tools/virsh.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index bad78c9..423e2d8 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1101,11 +1101,18 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const
char *name,
if (VIR_STRDUP(*optstr, value + 1) < 0)
goto cleanup;
}
+ *opts_seen |= 1 << i;
continue;
}
if ((*opts_seen & (1 << i)) && opt->type !=
VSH_OT_ARGV) {
- vshError(ctl, _("option --%s already seen"), name);
- goto cleanup;
+ if ((*opts_seen & (1 << (i - 1)))) {
+ vshError(ctl, _("option '--%s' duplicates its alias
'--%s'"),
+ cmd->opts[i].name, cmd->opts[i-1].name);
This is not right. This code depends on the aliased option being right
before the target option in the array describing the options. If you
move the options in the array around you may get strange error messages.
+ goto cleanup;
+ } else {
+ vshError(ctl, _("option '--%s' already seen"),
name);
+ goto cleanup;
+ }
}
*opts_seen |= 1 << i;
*opt_index = i;
The idea is good, but you need to make sure that the code doesn't depend
on ordering of the options in the array.
Peter
8:59 a.m.
On 10/30/2013 07:41 AM, Peter Krempa wrote:
> if ((*opts_seen & (1 << i)) &&
opt->type != VSH_OT_ARGV) {
> - vshError(ctl, _("option --%s already seen"), name);
> - goto cleanup;
> + if ((*opts_seen & (1 << (i - 1)))) {
> + vshError(ctl, _("option '--%s' duplicates its alias
'--%s'"),
> + cmd->opts[i].name, cmd->opts[i-1].name);
This is not right. This code depends on the aliased option being right
before the target option in the array describing the options. If you
move the options in the array around you may get strange error messages.
We already have a check baked into vshCmddefOptParse that guarantees
that all alias options appear in the list earlier than what the alias
expands to, so the rest of the code base is free to rely on that.
The idea is good, but you need to make sure that the code doesn't depend
on ordering of the options in the array.
I haven't looked closely at the patch, but we CAN depend on the order of
options within the array.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:04 a.m.
On 10/30/13 14:59, Eric Blake wrote:
On 10/30/2013 07:41 AM, Peter Krempa wrote:
>> if ((*opts_seen & (1 << i)) && opt->type !=
VSH_OT_ARGV) {
>> - vshError(ctl, _("option --%s already seen"), name);
>> - goto cleanup;
>> + if ((*opts_seen & (1 << (i - 1)))) {
>> + vshError(ctl, _("option '--%s' duplicates its
alias '--%s'"),
>> + cmd->opts[i].name, cmd->opts[i-1].name);
>
> This is not right. This code depends on the aliased option being right
> before the target option in the array describing the options. If you
> move the options in the array around you may get strange error messages.
We already have a check baked into vshCmddefOptParse that guarantees
that all alias options appear in the list earlier than what the alias
expands to, so the rest of the code base is free to rely on that.
>
> The idea is good, but you need to make sure that the code doesn't depend
> on ordering of the options in the array.
I haven't looked closely at the patch, but we CAN depend on the order of
options within the array.
Either way, offset of "-1" to the current option can't be guaranteed in
case we have for example multiple aliases for the same main command or
just put some in between, when the aliases will be before the command
but not _RIGHT_ before. (I actually tried it.)
Peter
6:26 p.m.
-----Original Message-----
From: Peter Krempa [mailto:pkrempa@redhat.com]
Sent: Wednesday, October 30, 2013 10:04 PM
To: Eric Blake; Chen Hanxiao; libvir-list(a)redhat.com
Subject: Re: [libvirt] [PATCH]virsh: track alias option and improve error message
when option duplicate its alias
On 10/30/13 14:59, Eric Blake wrote:
> On 10/30/2013 07:41 AM, Peter Krempa wrote:
>
>>> if ((*opts_seen & (1 << i)) && opt->type
!= VSH_OT_ARGV)
{
>>> - vshError(ctl, _("option --%s already seen"),
name);
>>> - goto cleanup;
>>> + if ((*opts_seen & (1 << (i - 1)))) {
>>> + vshError(ctl, _("option '--%s' duplicates
its alias
'--%s'"),
>>> + cmd->opts[i].name,
cmd->opts[i-1].name);
>>
>> This is not right. This code depends on the aliased option being right
>> before the target option in the array describing the options. If you
>> move the options in the array around you may get strange error messages.
>
> We already have a check baked into vshCmddefOptParse that guarantees
> that all alias options appear in the list earlier than what the alias
> expands to, so the rest of the code base is free to rely on that.
>
>>
>> The idea is good, but you need to make sure that the code doesn't depend
>> on ordering of the options in the array.
>
> I haven't looked closely at the patch, but we CAN depend on the order of
> options within the array.
Either way, offset of "-1" to the current option can't be guaranteed in
case we have for example multiple aliases for the same main command or
just put some in between, when the aliases will be before the command
but not _RIGHT_ before. (I actually tried it.)
Thanks. v2 patch will solve this issue.
Peter
4043
days inactive
4043
days old
4 comments
3 participants
participants (3)
-
Chen Hanxiao -
Eric Blake -
Peter Krempa