On Fri, Sep 24, 2021 at 11:03 AM Michal Prívozník <mprivozn@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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.
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.
