[libvirt] [PATCH RFC] test_driver: check that the domain is running in testDomainGetTime

[Michal Privoznik]

On 6/20/19 1:41 PM, Ilias Stamatis wrote:
> Signed-off-by: Ilias Stamatis <stamatis.iliass at gmail.com>
> ---
> 
> Currently in the test driver in APIs that would normally require guest
> agents and similar we are just checking if the domain is active (using
> virDomainObjCheckActive).
> 
> But a domain will be active even if stopped, so I would say that most of
> the time this is not sufficient since we really want to now that the
> domain is actually running, not that it's just active.
> 
> So something like what I include in this patch is needed instead.
> 
> I wonder if it would make sense to add a new function such as
> testDomainAgentAvailable inspired by the QEMU driver.
> 
> So the check whether the domain is actually running or not could be done
> there.
> 
> Also, in that case I'm not sure if calling both virDomainObjCheckActive
> and testDomainAgentAvailable is needed. I feel like calling the second
> one only is sufficient. However, in the QEMU driver always both are
> called.
> 
> Should I go ahead with implementing the testDomainAgentAvailable
> function and search for all the places in the test driver that it should
> be used instead of virDomainObjCheckActive?
> 
>   src/test/test_driver.c | 20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> index 2a0ffbc6c5..8e5bd4e670 100755
> --- a/src/test/test_driver.c
> +++ b/src/test/test_driver.c
> @@ -1984,17 +1984,33 @@ testDomainGetState(virDomainPtr domain,
>   }
> 
>   static int
> -testDomainGetTime(virDomainPtr dom ATTRIBUTE_UNUSED,
> +testDomainGetTime(virDomainPtr dom,
>                     long long *seconds,
>                     unsigned int *nseconds,
>                     unsigned int flags)
>   {
> +    int ret = -1;
> +    virDomainObjPtr vm = NULL;

Personal preference, I like @ret to be the last variable.

> +
>       virCheckFlags(0, -1);
> 
> +    if (!(vm = testDomObjFromDomain(dom)))
> +        goto cleanup;

Again, nit pick s/goto cleanup/return -1/ ;-)

> +
> +    if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("domain is not running"));
> +        goto cleanup;
> +    }
> +
>       *seconds = 627319920;
>       *nseconds = 0;
> 
> -    return 0;
> +    ret = 0;
> +
> + cleanup:
> +    virDomainObjEndAPI(&vm);
> +    return ret;
>   }
> 

ACKed and pushed.

Michal