* [PATCH] xl: fix incorrect display of illegal option character
@ 2011-01-28 8:49 Andre Przywara
2011-01-28 17:57 ` Ian Jackson
0 siblings, 1 reply; 4+ messages in thread
From: Andre Przywara @ 2011-01-28 8:49 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel
[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]
Hi,
according to the getopt(3) manpage (and to my testing) getopt returns
'?' if an unknown option character is found and stores the insulting
character in optopt.
This patch fixes the broken output in such a situation:
root@dosorca:/data/images# xl vcpu-list -j
option `?' not supported.
Name ID VCPU CPU State Time(s) CPU Affinity
Domain-0 0 0 0 -b- 193.1 any cpu
turns into:
root@dosorca:/data/images# xl vcpu-list -j
option `j' not supported.
Name ID VCPU CPU State Time(s) CPU Affinity
Domain-0 0 0 0 -b- 193.1 any cpu
Please apply to 4.1.0-rc.
By the way: some command parsers totally omit the faulting character and
just output "option not supported."
Would you still accept a patch which turns all of those occurrences into
the upper, more verbose form?
Especially as those conditions are not considered fatal and the command
execution continues anyway, I found it rather confusing to read an
unspecific error message without giving me a clue what I did wrong.
Regards,
Andre.
Signed-off-by: Andre Przywara <andre.przywara@amd.com>
--
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
[-- Attachment #2: xl_fix_illegal_char_display.patch --]
[-- Type: text/x-patch, Size: 11197 bytes --]
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 6341767..8e713be 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2955,7 +2955,7 @@ int main_dump_core(int argc, char **argv)
help("dump-core");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3462,7 +3462,7 @@ int main_vcpulist(int argc, char **argv)
help("vcpu-list");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3553,7 +3553,7 @@ int main_vcpupin(int argc, char **argv)
help("vcpu-pin");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3598,7 +3598,7 @@ int main_vcpuset(int argc, char **argv)
help("vcpu-set");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3762,7 +3762,7 @@ int main_info(int argc, char **argv)
numa = 1;
break;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3834,7 +3834,7 @@ int main_sched_credit(int argc, char **argv)
help("sched-credit");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3893,7 +3893,7 @@ int main_domid(int argc, char **argv)
help("domid");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3927,7 +3927,7 @@ int main_domname(int argc, char **argv)
help("domname");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3968,7 +3968,7 @@ int main_rename(int argc, char **argv)
help("rename");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4005,7 +4005,7 @@ int main_trigger(int argc, char **argv)
help("trigger");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4046,7 +4046,7 @@ int main_sysrq(int argc, char **argv)
help("sysrq");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4146,7 +4146,7 @@ int main_top(int argc, char **argv)
help("top");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4169,7 +4169,7 @@ int main_networkattach(int argc, char **argv)
help("network-attach");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4250,7 +4250,7 @@ int main_networklist(int argc, char **argv)
help("network-list");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4299,7 +4299,7 @@ int main_networkdetach(int argc, char **argv)
help("network-detach");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4345,7 +4345,7 @@ int main_blockattach(int argc, char **argv)
help("block-attach");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4418,7 +4418,7 @@ int main_blocklist(int argc, char **argv)
help("block-list");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4464,7 +4464,7 @@ int main_blockdetach(int argc, char **argv)
help("block-detach");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4503,7 +4503,7 @@ int main_network2attach(int argc, char **argv)
help("network2-attach");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4596,7 +4596,7 @@ int main_network2list(int argc, char **argv)
help("network2-list");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4640,7 +4640,7 @@ int main_network2detach(int argc, char **argv)
help("network2-detach");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4839,7 +4839,7 @@ int main_uptime(int argc, char **argv)
help("uptime");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4874,7 +4874,7 @@ int main_tmem_list(int argc, char **argv)
help("tmem-list");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4915,7 +4915,7 @@ int main_tmem_freeze(int argc, char **argv)
help("tmem-freeze");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4951,7 +4951,7 @@ int main_tmem_destroy(int argc, char **argv)
help("tmem-destroy");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4987,7 +4987,7 @@ int main_tmem_thaw(int argc, char **argv)
help("tmem-thaw");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -5037,7 +5037,7 @@ int main_tmem_set(int argc, char **argv)
help("tmem-set");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -5095,7 +5095,7 @@ int main_tmem_shared_auth(int argc, char **argv)
help("tmem-shared-auth");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -5140,7 +5140,7 @@ int main_tmem_freeable(int argc, char **argv)
help("tmem-freeable");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -5468,7 +5468,7 @@ int main_cpupooldestroy(int argc, char **argv)
help("cpupool-destroy");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -5502,7 +5502,7 @@ int main_cpupoolrename(int argc, char **argv)
help("cpupool-rename");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -5545,7 +5545,7 @@ int main_cpupoolcpuadd(int argc, char **argv)
help("cpupool-cpu-add");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -5608,7 +5608,7 @@ int main_cpupoolcpuremove(int argc, char **argv)
help("cpupool-cpu-remove");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -5670,7 +5670,7 @@ int main_cpupoolmigrate(int argc, char **argv)
help("cpupool-migrate");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -5727,7 +5727,7 @@ int main_cpupoolnumasplit(int argc, char **argv)
help("cpupool-numa-split");
return 0;
default:
- fprintf(stderr, "option `%c' not supported.\n", opt);
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] xl: fix incorrect display of illegal option character
2011-01-28 8:49 [PATCH] xl: fix incorrect display of illegal option character Andre Przywara
@ 2011-01-28 17:57 ` Ian Jackson
2011-01-28 22:53 ` [PATCH] xl: output " Andre Przywara
0 siblings, 1 reply; 4+ messages in thread
From: Ian Jackson @ 2011-01-28 17:57 UTC (permalink / raw)
To: Andre Przywara; +Cc: xen-devel
Andre Przywara writes ("[PATCH] xl: fix incorrect display of illegal option character"):
> according to the getopt(3) manpage (and to my testing) getopt returns
> '?' if an unknown option character is found and stores the insulting
> character in optopt.
> This patch fixes the broken output in such a situation:
...
> Please apply to 4.1.0-rc.
Done, thanks.
> By the way: some command parsers totally omit the faulting character and
> just output "option not supported."
> Would you still accept a patch which turns all of those occurrences into
> the upper, more verbose form?
Hrm, I'm not sure. I guess if the patch is very very obvious.
> Especially as those conditions are not considered fatal and the command
> execution continues anyway, I found it rather confusing to read an
> unspecific error message without giving me a clue what I did wrong.
TBH I think it is a bug when unknown options are not fatal. But
it's probably not sensible to change this now.
Ian.
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH] xl: output illegal option character
2011-01-28 17:57 ` Ian Jackson
@ 2011-01-28 22:53 ` Andre Przywara
2011-02-01 19:06 ` Ian Jackson
0 siblings, 1 reply; 4+ messages in thread
From: Andre Przywara @ 2011-01-28 22:53 UTC (permalink / raw)
To: Ian Jackson; +Cc: xen-devel
[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]
On 01/28/2011 06:57 PM, Ian Jackson wrote:
>> By the way: some command parsers totally omit the faulting character and
>> just output "option not supported."
>> Would you still accept a patch which turns all of those occurrences into
>> the upper, more verbose form?
>
> Hrm, I'm not sure. I guess if the patch is very very obvious.
What do you think about the attached one? Looks quite obvious to me.
>
>> Especially as those conditions are not considered fatal and the command
>> execution continues anyway, I found it rather confusing to read an
>> unspecific error message without giving me a clue what I did wrong.
>
> TBH I think it is a bug when unknown options are not fatal. But
> it's probably not sensible to change this now.
Looking at the two patches I hope I can refactor the option parsing into
common code after the release. This should be easier to fix then.
Regards,
Andre.
----------
Though illegal characters on xl command lines are catched, the user
isn't currently informed which one was not right.
This patch fixes this by printing the faulting character.
Signed-off-by: Andre Przywara <andre.przywara@amd.com>
--
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
[-- Attachment #2: xl_output_illegal_character.patch --]
[-- Type: text/plain, Size: 7070 bytes --]
diff -r 52e928af3637 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c Fri Jan 28 19:37:49 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c Fri Jan 28 23:40:38 2011 +0100
@@ -1762,7 +1762,7 @@
help("mem-max");
exit(0);
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -1809,7 +1809,7 @@
help("mem-set");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -1875,7 +1875,7 @@
help("cd-eject");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -1903,7 +1903,7 @@
help("cd-insert");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -1944,7 +1944,7 @@
num = atoi(optarg);
break;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -1993,7 +1993,7 @@
help("vncviewer");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -2032,7 +2032,7 @@
help("pci-list-assignable-devices");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -2069,7 +2069,7 @@
help("pci-list");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -2114,7 +2114,7 @@
force = 1;
break;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -2155,7 +2155,7 @@
help("pci-attach");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -2816,7 +2816,7 @@
help("restore");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -2865,7 +2865,7 @@
debug = 1;
break;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -2894,7 +2894,7 @@
help("save");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -2938,7 +2938,7 @@
debug = 1;
break;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -2998,7 +2998,7 @@
help("pause");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3025,7 +3025,7 @@
help("unpause");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3051,7 +3051,7 @@
help("destroy");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3081,7 +3081,7 @@
wait = 1;
break;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3107,7 +3107,7 @@
help("reboot");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3153,7 +3153,7 @@
verbose = 1;
break;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3204,7 +3204,7 @@
help("list-vm");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3262,7 +3262,7 @@
quiet = 1;
break;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -3328,7 +3328,7 @@
help("button-press");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4103,7 +4103,7 @@
help("debug-keys");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -4138,7 +4138,7 @@
help("dmesg");
return 0;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -5219,7 +5219,7 @@
dryrun = 1;
break;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
@@ -5416,7 +5416,7 @@
opt_cpus = 1;
break;
default:
- fprintf(stderr, "option not supported\n");
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
break;
}
}
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] xl: output illegal option character
2011-01-28 22:53 ` [PATCH] xl: output " Andre Przywara
@ 2011-02-01 19:06 ` Ian Jackson
0 siblings, 0 replies; 4+ messages in thread
From: Ian Jackson @ 2011-02-01 19:06 UTC (permalink / raw)
To: Andre Przywara; +Cc: xen-devel
Andre Przywara writes ("[PATCH] xl: output illegal option character"):
> What do you think about the attached one? Looks quite obvious to me.
Yes, applied, thanks.
> Looking at the two patches I hope I can refactor the option parsing into
> common code after the release. This should be easier to fix then.
That would be lovely.
Ian.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-02-01 19:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-01-28 8:49 [PATCH] xl: fix incorrect display of illegal option character Andre Przywara
2011-01-28 17:57 ` Ian Jackson
2011-01-28 22:53 ` [PATCH] xl: output " Andre Przywara
2011-02-01 19:06 ` Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.