[libvirt] [PATCH] virsh: fix keepalive error msg, man page update
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); exit(EXIT_FAILURE); } ctl->keepalive_interval = keepalive; @@ -3481,8 +3484,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-count"); + vshError(ctl, + _("option %s requires a positive integer argument " + "within range <0,%d>"), + longindex == -1 ? "-K" : "--keepalive-count", + INT_MAX); exit(EXIT_FAILURE); } ctl->keepalive_count = keepalive; diff --git a/tools/virsh.pod b/tools/virsh.pod index e0dfd13..9bee664 100644 --- a/tools/virsh.pod +++ 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 interval to 0 disables client keepalive mechanism. =item B<-K>, B<--keepalive-count> I<COUNT> Set a number of times keepalive message can be sent without getting an -answer from the server without marking the connection dead. There is +answer from the server without marking the connection dead. I<COUNT> +must be an integer value within range <0,INT_MAX>. There is no effect to this setting in case the I<INTERVAL> is set to 0. =item B<-l>, B<--log> I<FILE> -- 1.9.3
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); }
exit(EXIT_FAILURE); } ctl->keepalive_interval = keepalive; @@ -3481,8 +3484,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-count"); + vshError(ctl, + _("option %s requires a positive integer argument " + "within range <0,%d>"), + longindex == -1 ? "-K" : "--keepalive-count", + INT_MAX); exit(EXIT_FAILURE);
Similarly here.
} ctl->keepalive_count = keepalive; diff --git a/tools/virsh.pod b/tools/virsh.pod index e0dfd13..9bee664 100644 --- a/tools/virsh.pod +++ 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.
=item B<-K>, B<--keepalive-count> I<COUNT>
Set a number of times keepalive message can be sent without getting an -answer from the server without marking the connection dead. There is +answer from the server without marking the connection dead. I<COUNT> +must be an integer value within range <0,INT_MAX>. There is
Same here. Martin
no effect to this setting in case the I<INTERVAL> is set to 0.
=item B<-l>, B<--log> I<FILE> -- 1.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
participants (3)
-
Eric Blake -
Erik Skultety -
Martin Kletzander