Message ID | 20170802201900.11890-1-eblake@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 08/02/2017 03:19 PM, Eric Blake wrote: > Back when the test was introduced, in commit 62c39b307, the > test was set up to run qemu-ga directly on the host performing > the test, and defaults to limiting itself to safe commands. At > the time, it was envisioned that setting QGA_TEST_SIDE_EFFECTING > in the environment could cover a few more commands, while noting > the potential danger of those side effects running in the host. > Rather than leave this untested time-bomb in place, rip it out. Oh, I meant to add that I'm wondering if this should be considered 2.10 material.
----- Original Message ----- > Back when the test was introduced, in commit 62c39b307, the > test was set up to run qemu-ga directly on the host performing > the test, and defaults to limiting itself to safe commands. At > the time, it was envisioned that setting QGA_TEST_SIDE_EFFECTING > in the environment could cover a few more commands, while noting > the potential danger of those side effects running in the host. > > But this has NEVER been tested: if you enable the environment > variable, the test WILL fail. One obvious reason: if you are not > running as root, you'll probably get a permission failure when > trying to freeze the file systems, or when changing system time. > Less obvious: if you run the test as root (wow, you're brave), you > could end up hanging if the test tries to log things to a > temporarily frozen filesystem. But the cutest reason of all: if > you get past the above hurdles, the test uses invalid JSON in > test_qga_fstrim() (missing '' around the dictionary key 'minimum'), > and will thus fail an assertion in qmp_fd(). > > Rather than leave this untested time-bomb in place, rip it out. > Hopefully, as originally envisioned, we can find an opportunity > to test an actual sandboxed guest where the guest-agent has > full permissions and will not unduly affect the host running > the test - if so, 'git revert' can be used if desired, for > salvaging any useful parts of this attempt. > > Signed-off-by: Eric Blake <eblake@redhat.com> I think it used to work in a vm (as qemu-ga user), but I don't mind to revert it if we need it back: Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > tests/test-qga.c | 90 > -------------------------------------------------------- > 1 file changed, 90 deletions(-) > > diff --git a/tests/test-qga.c b/tests/test-qga.c > index 06783e7585..fd6bc7690f 100644 > --- a/tests/test-qga.c > +++ b/tests/test-qga.c > @@ -642,65 +642,6 @@ static void test_qga_get_time(gconstpointer fix) > QDECREF(ret); > } > > -static void test_qga_set_time(gconstpointer fix) > -{ > - const TestFixture *fixture = fix; > - QDict *ret; > - int64_t current, time; > - gchar *cmd; > - > - /* get current time */ > - ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-time'}"); > - g_assert_nonnull(ret); > - qmp_assert_no_error(ret); > - current = qdict_get_int(ret, "return"); > - g_assert_cmpint(current, >, 0); > - QDECREF(ret); > - > - /* set some old time */ > - ret = qmp_fd(fixture->fd, "{'execute': 'guest-set-time'," > - " 'arguments': { 'time': 1000 } }"); > - g_assert_nonnull(ret); > - qmp_assert_no_error(ret); > - QDECREF(ret); > - > - /* check old time */ > - ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-time'}"); > - g_assert_nonnull(ret); > - qmp_assert_no_error(ret); > - time = qdict_get_int(ret, "return"); > - g_assert_cmpint(time / 1000, <, G_USEC_PER_SEC * 10); > - QDECREF(ret); > - > - /* set back current time */ > - cmd = g_strdup_printf("{'execute': 'guest-set-time'," > - " 'arguments': { 'time': %" PRId64 " } }", > - current + time * 1000); > - ret = qmp_fd(fixture->fd, cmd); > - g_free(cmd); > - g_assert_nonnull(ret); > - qmp_assert_no_error(ret); > - QDECREF(ret); > -} > - > -static void test_qga_fstrim(gconstpointer fix) > -{ > - const TestFixture *fixture = fix; > - QDict *ret; > - QList *list; > - const QListEntry *entry; > - > - ret = qmp_fd(fixture->fd, "{'execute': 'guest-fstrim'," > - " arguments: { minimum: 4194304 } }"); > - g_assert_nonnull(ret); > - qmp_assert_no_error(ret); > - list = qdict_get_qlist(ret, "return"); > - entry = qlist_first(list); > - g_assert(qdict_haskey(qobject_to_qdict(entry->value), "paths")); > - > - QDECREF(ret); > -} > - > static void test_qga_blacklist(gconstpointer data) > { > TestFixture fix; > @@ -831,30 +772,6 @@ static void test_qga_fsfreeze_status(gconstpointer fix) > QDECREF(ret); > } > > -static void test_qga_fsfreeze_and_thaw(gconstpointer fix) > -{ > - const TestFixture *fixture = fix; > - QDict *ret; > - const gchar *status; > - > - ret = qmp_fd(fixture->fd, "{'execute': 'guest-fsfreeze-freeze'}"); > - g_assert_nonnull(ret); > - qmp_assert_no_error(ret); > - QDECREF(ret); > - > - ret = qmp_fd(fixture->fd, "{'execute': 'guest-fsfreeze-status'}"); > - g_assert_nonnull(ret); > - qmp_assert_no_error(ret); > - status = qdict_get_try_str(ret, "return"); > - g_assert_cmpstr(status, ==, "frozen"); > - QDECREF(ret); > - > - ret = qmp_fd(fixture->fd, "{'execute': 'guest-fsfreeze-thaw'}"); > - g_assert_nonnull(ret); > - qmp_assert_no_error(ret); > - QDECREF(ret); > -} > - > static void test_qga_guest_exec(gconstpointer fix) > { > const TestFixture *fixture = fix; > @@ -1029,13 +946,6 @@ int main(int argc, char **argv) > g_test_add_data_func("/qga/guest-get-osinfo", &fix, > test_qga_guest_get_osinfo); > > - if (g_getenv("QGA_TEST_SIDE_EFFECTING")) { > - g_test_add_data_func("/qga/fsfreeze-and-thaw", &fix, > - test_qga_fsfreeze_and_thaw); > - g_test_add_data_func("/qga/set-time", &fix, test_qga_set_time); > - g_test_add_data_func("/qga/fstrim", &fix, test_qga_fstrim); > - } > - > ret = g_test_run(); > > fixture_tear_down(&fix, NULL); > -- > 2.13.3 > >
On 08/02/2017 05:07 PM, Marc-André Lureau wrote: >> >> Rather than leave this untested time-bomb in place, rip it out. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> > > I think it used to work in a vm (as qemu-ga user), but I don't mind to revert it if we need it back: > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> Should this be R-b instead of S-o-b?
Hi ----- Original Message ----- > On 08/02/2017 05:07 PM, Marc-André Lureau wrote: > > >> > >> Rather than leave this untested time-bomb in place, rip it out. > > >> > >> Signed-off-by: Eric Blake <eblake@redhat.com> > > > > I think it used to work in a vm (as qemu-ga user), but I don't mind to > > revert it if we need it back: > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Should this be R-b instead of S-o-b? correct, Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
diff --git a/tests/test-qga.c b/tests/test-qga.c index 06783e7585..fd6bc7690f 100644 --- a/tests/test-qga.c +++ b/tests/test-qga.c @@ -642,65 +642,6 @@ static void test_qga_get_time(gconstpointer fix) QDECREF(ret); } -static void test_qga_set_time(gconstpointer fix) -{ - const TestFixture *fixture = fix; - QDict *ret; - int64_t current, time; - gchar *cmd; - - /* get current time */ - ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-time'}"); - g_assert_nonnull(ret); - qmp_assert_no_error(ret); - current = qdict_get_int(ret, "return"); - g_assert_cmpint(current, >, 0); - QDECREF(ret); - - /* set some old time */ - ret = qmp_fd(fixture->fd, "{'execute': 'guest-set-time'," - " 'arguments': { 'time': 1000 } }"); - g_assert_nonnull(ret); - qmp_assert_no_error(ret); - QDECREF(ret); - - /* check old time */ - ret = qmp_fd(fixture->fd, "{'execute': 'guest-get-time'}"); - g_assert_nonnull(ret); - qmp_assert_no_error(ret); - time = qdict_get_int(ret, "return"); - g_assert_cmpint(time / 1000, <, G_USEC_PER_SEC * 10); - QDECREF(ret); - - /* set back current time */ - cmd = g_strdup_printf("{'execute': 'guest-set-time'," - " 'arguments': { 'time': %" PRId64 " } }", - current + time * 1000); - ret = qmp_fd(fixture->fd, cmd); - g_free(cmd); - g_assert_nonnull(ret); - qmp_assert_no_error(ret); - QDECREF(ret); -} - -static void test_qga_fstrim(gconstpointer fix) -{ - const TestFixture *fixture = fix; - QDict *ret; - QList *list; - const QListEntry *entry; - - ret = qmp_fd(fixture->fd, "{'execute': 'guest-fstrim'," - " arguments: { minimum: 4194304 } }"); - g_assert_nonnull(ret); - qmp_assert_no_error(ret); - list = qdict_get_qlist(ret, "return"); - entry = qlist_first(list); - g_assert(qdict_haskey(qobject_to_qdict(entry->value), "paths")); - - QDECREF(ret); -} - static void test_qga_blacklist(gconstpointer data) { TestFixture fix; @@ -831,30 +772,6 @@ static void test_qga_fsfreeze_status(gconstpointer fix) QDECREF(ret); } -static void test_qga_fsfreeze_and_thaw(gconstpointer fix) -{ - const TestFixture *fixture = fix; - QDict *ret; - const gchar *status; - - ret = qmp_fd(fixture->fd, "{'execute': 'guest-fsfreeze-freeze'}"); - g_assert_nonnull(ret); - qmp_assert_no_error(ret); - QDECREF(ret); - - ret = qmp_fd(fixture->fd, "{'execute': 'guest-fsfreeze-status'}"); - g_assert_nonnull(ret); - qmp_assert_no_error(ret); - status = qdict_get_try_str(ret, "return"); - g_assert_cmpstr(status, ==, "frozen"); - QDECREF(ret); - - ret = qmp_fd(fixture->fd, "{'execute': 'guest-fsfreeze-thaw'}"); - g_assert_nonnull(ret); - qmp_assert_no_error(ret); - QDECREF(ret); -} - static void test_qga_guest_exec(gconstpointer fix) { const TestFixture *fixture = fix; @@ -1029,13 +946,6 @@ int main(int argc, char **argv) g_test_add_data_func("/qga/guest-get-osinfo", &fix, test_qga_guest_get_osinfo); - if (g_getenv("QGA_TEST_SIDE_EFFECTING")) { - g_test_add_data_func("/qga/fsfreeze-and-thaw", &fix, - test_qga_fsfreeze_and_thaw); - g_test_add_data_func("/qga/set-time", &fix, test_qga_set_time); - g_test_add_data_func("/qga/fstrim", &fix, test_qga_fstrim); - } - ret = g_test_run(); fixture_tear_down(&fix, NULL);
Back when the test was introduced, in commit 62c39b307, the test was set up to run qemu-ga directly on the host performing the test, and defaults to limiting itself to safe commands. At the time, it was envisioned that setting QGA_TEST_SIDE_EFFECTING in the environment could cover a few more commands, while noting the potential danger of those side effects running in the host. But this has NEVER been tested: if you enable the environment variable, the test WILL fail. One obvious reason: if you are not running as root, you'll probably get a permission failure when trying to freeze the file systems, or when changing system time. Less obvious: if you run the test as root (wow, you're brave), you could end up hanging if the test tries to log things to a temporarily frozen filesystem. But the cutest reason of all: if you get past the above hurdles, the test uses invalid JSON in test_qga_fstrim() (missing '' around the dictionary key 'minimum'), and will thus fail an assertion in qmp_fd(). Rather than leave this untested time-bomb in place, rip it out. Hopefully, as originally envisioned, we can find an opportunity to test an actual sandboxed guest where the guest-agent has full permissions and will not unduly affect the host running the test - if so, 'git revert' can be used if desired, for salvaging any useful parts of this attempt. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/test-qga.c | 90 -------------------------------------------------------- 1 file changed, 90 deletions(-)