Re: [libvirt] [PATCH] virsh: fix keepalive error msg, man page update

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); }

> +++ 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.