[libvirt] [PATCH] Check that virsh -d argument is numeric
Having been bitten one more time by the use of -d to pass the hypervisor URI instead of -c (confusion coming from CVS using -d to specify the root), I suggest to drop atoi and use the function with checking and error out with proper explanation instead of silently failing ! paphio:~/libvirt -> /usr/bin/virsh -d qemu+ssh://test2/system list Id Name State ---------------------------------- paphio:~/libvirt -> tools/virsh -d qemu+ssh://test2/system list error: option -d take a numeric argument paphio:~/libvirt -> tools/virsh -d 5 list command: "list " Id Name State ---------------------------------- paphio:~/libvirt -> Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 08/02/2010 09:30 AM, Daniel Veillard wrote:
Having been bitten one more time by the use of -d to pass the hypervisor URI instead of -c (confusion coming from CVS using -d to specify the root), I suggest to drop atoi and use the function with checking and error out with proper explanation instead of silently failing !
Hear hear - atoi() is inherently stupid. There's a 'make syntax-check' that we could turn on to prohibit all use of atoi(), but I don't know if the code base is ready for that, so I'll save it for another patch on another day.
paphio:~/libvirt -> tools/virsh -d qemu+ssh://test2/system list error: option -d take a numeric argument
+ if (virStrToLong_i(optarg, NULL, 10, &ctl->debug) < 0) { Any reason you are still requiring decimal, or would it make more sense to branch out and s/10/0/ in order to also accept hex? At any rate, ACK to the patch. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Aug 02, 2010 at 12:19:54PM -0600, Eric Blake wrote:
On 08/02/2010 09:30 AM, Daniel Veillard wrote:
Having been bitten one more time by the use of -d to pass the hypervisor URI instead of -c (confusion coming from CVS using -d to specify the root), I suggest to drop atoi and use the function with checking and error out with proper explanation instead of silently failing !
Hear hear - atoi() is inherently stupid. There's a 'make syntax-check' that we could turn on to prohibit all use of atoi(), but I don't know if the code base is ready for that, so I'll save it for another patch on another day.
let's try to clean this up after 0.8.3
paphio:~/libvirt -> tools/virsh -d qemu+ssh://test2/system list error: option -d take a numeric argument
+ if (virStrToLong_i(optarg, NULL, 10, &ctl->debug) < 0) {
Any reason you are still requiring decimal, or would it make more sense to branch out and s/10/0/ in order to also accept hex?
the other use in virsh was also expecting decimal, I guess for a debug level it's not a big deal to stick to decimal :-)
At any rate, ACK to the patch.
Okay, pushed :-) thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 08/02/10 - 09:22:26PM, Daniel Veillard wrote:
On Mon, Aug 02, 2010 at 12:19:54PM -0600, Eric Blake wrote:
On 08/02/2010 09:30 AM, Daniel Veillard wrote:
Having been bitten one more time by the use of -d to pass the hypervisor URI instead of -c (confusion coming from CVS using -d to specify the root), I suggest to drop atoi and use the function with checking and error out with proper explanation instead of silently failing !
Hear hear - atoi() is inherently stupid. There's a 'make syntax-check' that we could turn on to prohibit all use of atoi(), but I don't know if the code base is ready for that, so I'll save it for another patch on another day.
let's try to clean this up after 0.8.3
Yeah, agreed. There aren't a huge number of uses of atoi() in the codebase, but there are enough to require a bit of churn. Best left until after the release. -- Chris Lalancette
On 08/02/2010 09:30 AM, Daniel Veillard wrote:
Having been bitten one more time by the use of -d to pass the hypervisor URI instead of -c (confusion coming from CVS using -d to specify the root), I suggest to drop atoi and use the function with checking and error out with proper explanation instead of silently failing !
+ vshError(ctl, _("option -d takes a numeric argument"), arg);
Serves me right for not noticing this; gcc complained that the printf string doesn't match the number of arguments. I'm pushing this trivial followup: diff --git i/tools/virsh.c w/tools/virsh.c index 926652a..cedfb5a 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -11022,7 +11022,7 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) switch (arg) { case 'd': if (virStrToLong_i(optarg, NULL, 10, &ctl->debug) < 0) { - vshError(ctl, _("option -d takes a numeric argument"), arg); + vshError(ctl, _("option -d takes a numeric argument: %s"), arg); exit(EXIT_FAILURE); } break; -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 08/02/2010 01:49 PM, Eric Blake wrote:
On 08/02/2010 09:30 AM, Daniel Veillard wrote:
Having been bitten one more time by the use of -d to pass the hypervisor URI instead of -c (confusion coming from CVS using -d to specify the root), I suggest to drop atoi and use the function with checking and error out with proper explanation instead of silently failing !
+ vshError(ctl, _("option -d takes a numeric argument"), arg);
Serves me right for not noticing this; gcc complained that the printf string doesn't match the number of arguments.
I'm pushing this trivial followup:
diff --git i/tools/virsh.c w/tools/virsh.c index 926652a..cedfb5a 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -11022,7 +11022,7 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) switch (arg) { case 'd': if (virStrToLong_i(optarg, NULL, 10, &ctl->debug) < 0) { - vshError(ctl, _("option -d takes a numeric argument"), arg); + vshError(ctl, _("option -d takes a numeric argument: %s"), arg);
Or, better yet: + vshError(ctl, _("option -d takes a numeric argument)); since arg is not a string to be printed with %s. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Aug 02, 2010 at 01:57:36PM -0600, Eric Blake wrote:
On 08/02/2010 01:49 PM, Eric Blake wrote:
On 08/02/2010 09:30 AM, Daniel Veillard wrote:
Having been bitten one more time by the use of -d to pass the hypervisor URI instead of -c (confusion coming from CVS using -d to specify the root), I suggest to drop atoi and use the function with checking and error out with proper explanation instead of silently failing !
+ vshError(ctl, _("option -d takes a numeric argument"), arg);
Serves me right for not noticing this; gcc complained that the printf string doesn't match the number of arguments.
I'm pushing this trivial followup:
diff --git i/tools/virsh.c w/tools/virsh.c index 926652a..cedfb5a 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -11022,7 +11022,7 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) switch (arg) { case 'd': if (virStrToLong_i(optarg, NULL, 10, &ctl->debug) < 0) { - vshError(ctl, _("option -d takes a numeric argument"), arg); + vshError(ctl, _("option -d takes a numeric argument: %s"), arg);
Or, better yet:
+ vshError(ctl, _("option -d takes a numeric argument));
since arg is not a string to be printed with %s.
urgh ... I'm blind ! I commited too fast, 2 commits then ! thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Chris Lalancette -
Daniel Veillard -
Eric Blake