Signed-off-by: Ilias Stamatis <stamatis.iliass@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;
+
     virCheckFlags(0, -1);

+    if (!(vm = testDomObjFromDomain(dom)))
+        goto cleanup;
+
+    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;
 }

 #define TEST_SAVE_MAGIC "TestGuestMagic"
--
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 6/20/19 1:41 PM, Ilias Stamatis wrote:
> Signed-off-by: Ilias Stamatis <stamatis.iliass@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list