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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140827/f0230b89/attachment-0001.sig>