[Qemu-devel] [PATCH] tests: start a /qga/guest-exec test

[Qemu-devel] [PATCH] tests: start a /qga/guest-exec test

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Test a few guest-exec guest agent commands, added in qemu 2.5.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/test-qga.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/tests/test-qga.c b/tests/test-qga.c
index 72a89de..42c9a89 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -823,6 +823,75 @@ static void test_qga_fsfreeze_and_thaw(gconstpointer fix)
     QDECREF(ret);
 }
 
+static void test_qga_guest_exec(gconstpointer fix)
+{
+    const TestFixture *fixture = fix;
+    QDict *ret, *val, *error;
+    const gchar *class, *desc, *out;
+    gchar *decoded;
+    int64_t pid, now, exitcode;
+    gsize len;
+    bool exited;
+
+    /* exec 'echo foo bar' */
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
+                 " 'path': '/bin/echo', 'arg': [ 'foo', 'bar' ],"
+                 " 'capture-output': true } }");
+    g_assert_nonnull(ret);
+    qmp_assert_no_error(ret);
+    val = qdict_get_qdict(ret, "return");
+    pid = qdict_get_int(val, "pid");
+    g_assert_cmpint(pid, >, 0);
+    QDECREF(ret);
+
+    /* wait for completion */
+    now = g_get_monotonic_time();
+    do {
+        ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec-status',"
+                     " 'arguments': { 'pid': %" PRId64 "  } }", pid);
+        g_assert_nonnull(ret);
+        val = qdict_get_qdict(ret, "return");
+        exited = qdict_get_bool(val, "exited");
+        if (!exited) {
+            QDECREF(ret);
+        }
+    } while (!exited &&
+             g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
+    g_assert(exited);
+
+    /* check stdout */
+    exitcode = qdict_get_int(val, "exitcode");
+    g_assert_cmpint(exitcode, ==, 0);
+    out = qdict_get_str(val, "out-data");
+    decoded = g_base64_decode(out, &len);
+    g_assert_cmpint(len, ==, 8);
+    g_assert_cmpint(strncmp(decoded, "foo bar\n", 8), ==, 0);
+    g_free(decoded);
+    QDECREF(ret);
+
+    /* invalid command */
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
+                 " 'path': '/bin/invalid-cmd42' } }");
+    g_assert_nonnull(ret);
+    error = qdict_get_qdict(ret, "error");
+    class = qdict_get_try_str(error, "class");
+    desc = qdict_get_try_str(error, "desc");
+    g_assert_cmpstr(class, ==, "GenericError");
+    g_assert_cmpint(strlen(desc), >, 0);
+    QDECREF(ret);
+
+    /* invalid pid */
+    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec-status',"
+                 " 'arguments': { 'pid': 0 } }");
+    g_assert_nonnull(ret);
+    error = qdict_get_qdict(ret, "error");
+    class = qdict_get_try_str(error, "class");
+    desc = qdict_get_try_str(error, "desc");
+    g_assert_cmpstr(class, ==, "GenericError");
+    g_assert_cmpint(strlen(desc), >, 0);
+    QDECREF(ret);
+}
+
 int main(int argc, char **argv)
 {
     TestFixture fix;
@@ -853,6 +922,7 @@ int main(int argc, char **argv)
 
     g_test_add_data_func("/qga/blacklist", NULL, test_qga_blacklist);
     g_test_add_data_func("/qga/config", NULL, test_qga_config);
+    g_test_add_data_func("/qga/guest-exec", &fix, test_qga_guest_exec);
 
     if (g_getenv("QGA_TEST_SIDE_EFFECTING")) {
         g_test_add_data_func("/qga/fsfreeze-and-thaw", &fix,
-- 
2.7.4

Re: [Qemu-devel] [PATCH] tests: start a /qga/guest-exec test

[Michael Roth]

Quoting marcandre.lureau@redhat.com (2016-05-24 08:48:41)
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Test a few guest-exec guest agent commands, added in qemu 2.5.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/test-qga.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/tests/test-qga.c b/tests/test-qga.c
> index 72a89de..42c9a89 100644
> --- a/tests/test-qga.c
> +++ b/tests/test-qga.c
> @@ -823,6 +823,75 @@ static void test_qga_fsfreeze_and_thaw(gconstpointer fix)
>      QDECREF(ret);
>  }
> 
> +static void test_qga_guest_exec(gconstpointer fix)
> +{
> +    const TestFixture *fixture = fix;
> +    QDict *ret, *val, *error;
> +    const gchar *class, *desc, *out;
> +    gchar *decoded;
> +    int64_t pid, now, exitcode;
> +    gsize len;
> +    bool exited;
> +
> +    /* exec 'echo foo bar' */
> +    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
> +                 " 'path': '/bin/echo', 'arg': [ 'foo', 'bar' ],"

Maybe 'arg': [ '-n', '" test_str "' ]," to make the check below a little
more robust as well, as exercising that arguments are being passed
appropriately (since `echo "foo bar"` and `echo foo bar` are identical
as far as the output.

> +                 " 'capture-output': true } }");
> +    g_assert_nonnull(ret);
> +    qmp_assert_no_error(ret);
> +    val = qdict_get_qdict(ret, "return");
> +    pid = qdict_get_int(val, "pid");
> +    g_assert_cmpint(pid, >, 0);
> +    QDECREF(ret);
> +
> +    /* wait for completion */
> +    now = g_get_monotonic_time();
> +    do {
> +        ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec-status',"
> +                     " 'arguments': { 'pid': %" PRId64 "  } }", pid);
> +        g_assert_nonnull(ret);
> +        val = qdict_get_qdict(ret, "return");
> +        exited = qdict_get_bool(val, "exited");
> +        if (!exited) {
> +            QDECREF(ret);
> +        }
> +    } while (!exited &&
> +             g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
> +    g_assert(exited);
> +
> +    /* check stdout */
> +    exitcode = qdict_get_int(val, "exitcode");
> +    g_assert_cmpint(exitcode, ==, 0);
> +    out = qdict_get_str(val, "out-data");
> +    decoded = g_base64_decode(out, &len);
> +    g_assert_cmpint(len, ==, 8);
> +    g_assert_cmpint(strncmp(decoded, "foo bar\n", 8), ==, 0);
> +    g_free(decoded);
> +    QDECREF(ret);
> +
> +    /* invalid command */
> +    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
> +                 " 'path': '/bin/invalid-cmd42' } }");

Since there's no shared state between this and the above command I think
I'd rather this be in a separate test function. I know we have somewhat
of a precedence for this with test_qga_blacklist, but there's probably a
number of other checks worth adding that would eventually make this test
function a bit unwieldly.

> +    g_assert_nonnull(ret);
> +    error = qdict_get_qdict(ret, "error");
> +    class = qdict_get_try_str(error, "class");
> +    desc = qdict_get_try_str(error, "desc");
> +    g_assert_cmpstr(class, ==, "GenericError");
> +    g_assert_cmpint(strlen(desc), >, 0);
> +    QDECREF(ret);
> +
> +    /* invalid pid */
> +    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec-status',"
> +                 " 'arguments': { 'pid': 0 } }");
> +    g_assert_nonnull(ret);
> +    error = qdict_get_qdict(ret, "error");

g_assert_nonnull(error)

(qdict_get_try_str doesn't seem to check for null qdict)

> +    class = qdict_get_try_str(error, "class");
> +    desc = qdict_get_try_str(error, "desc");
> +    g_assert_cmpstr(class, ==, "GenericError");
> +    g_assert_cmpint(strlen(desc), >, 0);

I might be missing something in the error_setg path, but it doesn't seem
like we can rely on desc not being an empty string. guest-exec-status
doesn't use it in that fashion, but that's an internal detail rather
than something we can reliably test for.

Looks good otherwise.

> +    QDECREF(ret);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      TestFixture fix;
> @@ -853,6 +922,7 @@ int main(int argc, char **argv)
> 
>      g_test_add_data_func("/qga/blacklist", NULL, test_qga_blacklist);
>      g_test_add_data_func("/qga/config", NULL, test_qga_config);
> +    g_test_add_data_func("/qga/guest-exec", &fix, test_qga_guest_exec);
> 
>      if (g_getenv("QGA_TEST_SIDE_EFFECTING")) {
>          g_test_add_data_func("/qga/fsfreeze-and-thaw", &fix,
> -- 
> 2.7.4
> 

Re: [Qemu-devel] [PATCH] tests: start a /qga/guest-exec test

[Marc-André Lureau]

Hi

----- Original Message -----
> Quoting marcandre.lureau@redhat.com (2016-05-24 08:48:41)
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > Test a few guest-exec guest agent commands, added in qemu 2.5.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  tests/test-qga.c | 70
> >  ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> > 
> > diff --git a/tests/test-qga.c b/tests/test-qga.c
> > index 72a89de..42c9a89 100644
> > --- a/tests/test-qga.c
> > +++ b/tests/test-qga.c
> > @@ -823,6 +823,75 @@ static void test_qga_fsfreeze_and_thaw(gconstpointer
> > fix)
> >      QDECREF(ret);
> >  }
> > 
> > +static void test_qga_guest_exec(gconstpointer fix)
> > +{
> > +    const TestFixture *fixture = fix;
> > +    QDict *ret, *val, *error;
> > +    const gchar *class, *desc, *out;
> > +    gchar *decoded;
> > +    int64_t pid, now, exitcode;
> > +    gsize len;
> > +    bool exited;
> > +
> > +    /* exec 'echo foo bar' */
> > +    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
> > +                 " 'path': '/bin/echo', 'arg': [ 'foo', 'bar' ],"
> 
> Maybe 'arg': [ '-n', '" test_str "' ]," to make the check below a little
> more robust as well, as exercising that arguments are being passed
> appropriately (since `echo "foo bar"` and `echo foo bar` are identical
> as far as the output.
> 

Good suggestion, I had a similar idea but didn't try hard enough ;)

> > +                 " 'capture-output': true } }");
> > +    g_assert_nonnull(ret);
> > +    qmp_assert_no_error(ret);
> > +    val = qdict_get_qdict(ret, "return");
> > +    pid = qdict_get_int(val, "pid");
> > +    g_assert_cmpint(pid, >, 0);
> > +    QDECREF(ret);
> > +
> > +    /* wait for completion */
> > +    now = g_get_monotonic_time();
> > +    do {
> > +        ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec-status',"
> > +                     " 'arguments': { 'pid': %" PRId64 "  } }", pid);
> > +        g_assert_nonnull(ret);
> > +        val = qdict_get_qdict(ret, "return");
> > +        exited = qdict_get_bool(val, "exited");
> > +        if (!exited) {
> > +            QDECREF(ret);
> > +        }
> > +    } while (!exited &&
> > +             g_get_monotonic_time() < now + 5 * G_TIME_SPAN_SECOND);
> > +    g_assert(exited);
> > +
> > +    /* check stdout */
> > +    exitcode = qdict_get_int(val, "exitcode");
> > +    g_assert_cmpint(exitcode, ==, 0);
> > +    out = qdict_get_str(val, "out-data");
> > +    decoded = g_base64_decode(out, &len);
> > +    g_assert_cmpint(len, ==, 8);
> > +    g_assert_cmpint(strncmp(decoded, "foo bar\n", 8), ==, 0);
> > +    g_free(decoded);
> > +    QDECREF(ret);
> > +
> > +    /* invalid command */
> > +    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec', 'arguments': {"
> > +                 " 'path': '/bin/invalid-cmd42' } }");
> 
> Since there's no shared state between this and the above command I think
> I'd rather this be in a separate test function. I know we have somewhat
> of a precedence for this with test_qga_blacklist, but there's probably a
> number of other checks worth adding that would eventually make this test
> function a bit unwieldly.
> 

ok, moved to a "guest-exec-invalid" test

> > +    g_assert_nonnull(ret);
> > +    error = qdict_get_qdict(ret, "error");
> > +    class = qdict_get_try_str(error, "class");
> > +    desc = qdict_get_try_str(error, "desc");
> > +    g_assert_cmpstr(class, ==, "GenericError");
> > +    g_assert_cmpint(strlen(desc), >, 0);
> > +    QDECREF(ret);
> > +
> > +    /* invalid pid */
> > +    ret = qmp_fd(fixture->fd, "{'execute': 'guest-exec-status',"
> > +                 " 'arguments': { 'pid': 0 } }");
> > +    g_assert_nonnull(ret);
> > +    error = qdict_get_qdict(ret, "error");
> 
> g_assert_nonnull(error)
> 
> (qdict_get_try_str doesn't seem to check for null qdict)
> 

ok

> > +    class = qdict_get_try_str(error, "class");
> > +    desc = qdict_get_try_str(error, "desc");
> > +    g_assert_cmpstr(class, ==, "GenericError");
> > +    g_assert_cmpint(strlen(desc), >, 0);
> 
> I might be missing something in the error_setg path, but it doesn't seem
> like we can rely on desc not being an empty string. guest-exec-status
> doesn't use it in that fashion, but that's an internal detail rather
> than something we can reliably test for.
> 

It is set in qmp_build_error_object() from error_get_pretty() (err->msg). I am not sure from the schema if desc is mandatory or not, but I imagine it is as the class isn't specific enough.

> Looks good otherwise.

thanks for the review

> 
> > +    QDECREF(ret);
> > +}
> > +
> >  int main(int argc, char **argv)
> >  {
> >      TestFixture fix;
> > @@ -853,6 +922,7 @@ int main(int argc, char **argv)
> > 
> >      g_test_add_data_func("/qga/blacklist", NULL, test_qga_blacklist);
> >      g_test_add_data_func("/qga/config", NULL, test_qga_config);
> > +    g_test_add_data_func("/qga/guest-exec", &fix, test_qga_guest_exec);
> > 
> >      if (g_getenv("QGA_TEST_SIDE_EFFECTING")) {
> >          g_test_add_data_func("/qga/fsfreeze-and-thaw", &fix,
> > --
> > 2.7.4
> > 
> 
>