On 08/27/2014 07:39 AM, Martin Kletzander wrote: > On Wed, Aug 27, 2014 at 03:15:35PM +0200, Erik Skultety wrote: >> resolves https://bugzilla.redhat.com/show_bug.cgi?id=1132305 >> --- >> tools/virsh.c | 14 ++++++++++---- >> tools/virsh.pod | 6 ++++-- >> 2 files changed, 14 insertions(+), 6 deletions(-) >> >> diff --git a/tools/virsh.c b/tools/virsh.c >> index 30a84c1..f9b3991 100644 >> --- a/tools/virsh.c >> +++ b/tools/virsh.c >> @@ -3472,8 +3472,11 @@ vshParseArgv(vshControl *ctl, int argc, char >> **argv) >> case 'k': >> if (virStrToLong_i(optarg, NULL, 0, &keepalive) < 0 || >> keepalive < 0) { >> - vshError(ctl, _("option %s requires a positive >> numeric argument"), >> - longindex == -1 ? "-k" : >> "--keepalive-interval"); >> + vshError(ctl, >> + _("option %s requires a positive integer >> argument " >> + "within range <0,%d>"), >> + longindex == -1 ? "-k" : >> "--keepalive-interval", >> + INT_MAX); > > There is no reasonable use for any interval greater than, let's say, > 100 seconds (and that's already pretty extreme). INT_MAX seconds is > too much and reporting it may even confuse users. Imagine that we > would have to report the limits for *all* options. Why not just do 2 > conditions: > > if (virStrToLong_i(optarg, NULL, 0, &keepalive) < 0) { > vshError(ctl, _("Invalid value for option %s"), > longindex == -1 ? "-k" : "--keepalive-interval"); > exit(EXIT_FAILURE); > } > if (keepalive < 0) { > vshError(ctl, _("option %s requires a positive numeric argument"), > longindex == -1 ? "-k" : "--keepalive-interval"); > exit(EXIT_FAILURE); > } I like this better. >> +++ b/tools/virsh.pod >> @@ -77,13 +77,15 @@ given instead. >> =item B<-k>, B<--keepalive-interval> I<INTERVAL> >> >> Set an I<INTERVAL> (in seconds) for sending keepalive messages to >> -check whether connection to the server is still alive. Setting the >> +check whether connection to the server is still alive. I<INTERVAL> >> +must be an integer value within range <0,INT_MAX>. Setting the > > Same here, check another options that take "normal numbers", we don't > say that it needs to be between 0 and LLONG_MAX for example. > >> interval to 0 disables client keepalive mechanism. I think we can just drop the virsh.pod hunks; they aren't adding any useful information (I think the quoted BZ is asking for too much, merely because of the initial confusion on negative values). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list