On Fri, Sep 24, 2021 at 11:03 AM Michal Prívozník <mprivozn@redhat.com> wrote:

On 9/24/21 1:30 AM, Kristina Hanicova wrote:

...

Function returned false when everything ended successfully, there was a missing check for a return value. This patch fixes that.

Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f876f30cc5..2730acfba5 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11018,7 +11018,7 @@ cmdMigrateSetMaxDowntime(vshControl *ctl, const vshCmd *cmd) goto done; }

- if (virDomainMigrateSetMaxDowntime(dom, downtime, 0)) + if (virDomainMigrateSetMaxDowntime(dom, downtime, 0) < 0) goto done;

ret = true;

Technically, there wasn't any bug here. virDomainMigrateSetMaxDowntime() returns 0 on success and 0 won't evaluate to true thus 'goto done' is skipped over and ret is set to true.

And if anything else is returned (currently only -1 is possible) then i's evaluated to true and control jump to the done label. BUT, your fix is correct because in libvirt only negative values are treated as errors. Zero and positive values are treated as success. I'll reword the commit message a bit.

Sorry for not realizing this earlier.

Michal

I think the commit message could be rewritten to something as: If there was added a new return value indicating success to the function virDomainMigrateSetMaxDowntime() in the future, our function would treat it as an error state and would return false (indicating failure). This patch fixes it, so that the call of the function follows the same pattern as is currently set in libvirt.

On 9/24/21 1:30 AM, Kristina Hanicova wrote:

Signed-off-by: Kristina Hanicova <khanicov@redhat.com> --- tools/virsh-domain.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2730acfba5..cfa0016296 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11006,25 +11006,19 @@ cmdMigrateSetMaxDowntime(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshDomain) dom = NULL; unsigned long long downtime = 0; - bool ret = false;

if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false;

if (vshCommandOptULongLong(ctl, cmd, "downtime", &downtime) < 0) - goto done; + return false; + if (downtime < 1) { vshError(ctl, "%s", _("migrate: Invalid downtime")); - goto done; + return false; }

- if (virDomainMigrateSetMaxDowntime(dom, downtime, 0) < 0) - goto done; - - ret = true; - - done: - return ret; + return virDomainMigrateSetMaxDowntime(dom, downtime, 0) == 0; }

Honestly, I'm not big fan of return func() == 0 type of statements. But I think I can live with this. Michal