10:19 a.m.
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(a)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
6:41 a.m.
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(a)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(a)redhat.com>
I'll push in a bit...
Tks -
John
11:31 a.m.
John Ferlan <jferlan(a)redhat.com> wrote on 07/10/2017 06:41:34 AM:
From: John Ferlan <jferlan(a)redhat.com>
To: Scott Garfinkle <seg(a)us.ibm.com>, libvir-list(a)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
parameter
to
> the worker functions all have the correct (unsigned) type.
>
> Signed-off-by: Scott Garfinkle <seg(a)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
2682
days inactive
2704
days old
2 comments
2 participants
participants (2)
-
John Ferlan -
Scott Garfinkle