[libvirt] [PATCH] Use unsigned timeout in cmdMigrateSetMaxDowntime
While looking to implement a migrate-getmaxdowntime command (coming), I noticed that the setmaxdowntime is incorrectly looking at its parameter as a signed longlong. Not sure how that got past gcc, but here's a simple patch to make the command line parsing and the parameter to the worker functions all have the correct (unsigned) type. Signed-off-by: Scott Garfinkle <seg@us.ibm.com> --- tools/virsh-domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5311a57..0d21fe7 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10696,13 +10696,13 @@ static bool cmdMigrateSetMaxDowntime(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; - long long downtime = 0; + unsigned long long downtime = 0; bool ret = false; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; - if (vshCommandOptLongLong(ctl, cmd, "downtime", &downtime) < 0) + if (vshCommandOptULongLong(ctl, cmd, "downtime", &downtime) < 0) goto done; if (downtime < 1) { vshError(ctl, "%s", _("migrate: Invalid downtime")); -- 1.8.3.1
On 06/27/2017 11:19 AM, Scott Garfinkle wrote:
While looking to implement a migrate-getmaxdowntime command (coming), I noticed that the setmaxdowntime is incorrectly looking at its parameter as a signed longlong. Not sure how that got past gcc, but here's a simple patch to make the command line parsing and the parameter to the worker functions all have the correct (unsigned) type.
Signed-off-by: Scott Garfinkle <seg@us.ibm.com> --- tools/virsh-domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
"For some commands" allowing a -1 provides a mechanism to set an almost infinite time without having to type such a large value. Still in this case since, the "downtime < 1" check immediately follows it seems that wouldn't be the case here! Looking at QEMU code briefly - I do note the QEMU set downtime (and speed) commands that end up getting called are listed as "deprecated" in favor of migrate-set-parameters (downtime-limit and max-bandwidth, since QEMU 2.8). So while you're at thinking about a getmaxdowntime type functionality maybe you'd want to give that a go as well (of course you'd have to add capabilities to detect when it was implemented using set-parameters)... In any case, Reviewed-by: John Ferlan <jferlan@redhat.com> I'll push in a bit... Tks - John
From: John Ferlan <jferlan@redhat.com> To: Scott Garfinkle <seg@us.ibm.com>, libvir-list@redhat.com Date: 07/10/2017 06:41 AM Subject: Re: [libvirt] [PATCH] Use unsigned timeout in cmdMigrateSetMaxDowntime
On 06/27/2017 11:19 AM, Scott Garfinkle wrote:
While looking to implement a migrate-getmaxdowntime command (coming), I noticed that the setmaxdowntime is incorrectly looking at its parameter as a signed longlong. Not sure how that got past gcc, but here's a simple patch to make the command line parsing and the
John Ferlan <jferlan@redhat.com> wrote on 07/10/2017 06:41:34 AM: parameter to
the worker functions all have the correct (unsigned) type.
Signed-off-by: Scott Garfinkle <seg@us.ibm.com> --- tools/virsh-domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
"For some commands" allowing a -1 provides a mechanism to set an almost infinite time without having to type such a large value. Still in this case since, the "downtime < 1" check immediately follows it seems that wouldn't be the case here! Yes, and maybe removing that check would have been a better alternative? Still, thanks.
Looking at QEMU code briefly - I do note the QEMU set downtime (and speed) commands that end up getting called are listed as "deprecated" in favor of migrate-set-parameters (downtime-limit and max-bandwidth, since QEMU 2.8). So while you're at thinking about a getmaxdowntime type functionality maybe you'd want to give that a go as well (of course you'd have to add capabilities to detect when it was implemented using set-parameters)...
Thanks for pointing that out. So, I have patches to implement the get-maxdowntime, which would seem to be a separate but related effort. Being a newcomer to the code, I'll what are probably obvious questions: is the concern that qemu will eventually just stop providing that interface? Or, is there something inherently useful about changing the set-* commands? Otherwise, I'm not sure what the value to the code is of changing the already-working set commands. regards, Scott Garfinkle
participants (2)
-
John Ferlan -
Scott Garfinkle