[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] tests: start a /qga/guest-exec test
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH] tests: start a /qga/guest-exec test |
Date: |
Fri, 3 Jun 2016 07:31:02 -0400 (EDT) |
Hi
----- Original Message -----
> Quoting address@hidden (2016-05-24 08:48:41)
> > From: Marc-André Lureau <address@hidden>
> >
> > Test a few guest-exec guest agent commands, added in qemu 2.5.
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> > 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
> >
>
>