| Message ID | 1448382858-28616-2-git-send-email-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/24/2015 09:34 AM, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > This test exhibits a POSIX behaviour regarding switching between write > and read. It's undefined result if the application doesn't ensure a > flush between the two operations (with glibc, the flush can be implicit > when the buffer size is relatively small). The previous commit fixes > this test. > > Related to: > https://bugzilla.redhat.com/show_bug.cgi?id=1210246 > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > tests/test-qga.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 91 insertions(+) > > diff --git a/tests/test-qga.c b/tests/test-qga.c > index 6473846..6b6963e 100644 > --- a/tests/test-qga.c > +++ b/tests/test-qga.c > @@ -496,6 +496,96 @@ static void test_qga_file_ops(gconstpointer fix) > g_free(cmd); > } > > +static void test_qga_file_write_read(gconstpointer fix) > +{ > + const TestFixture *fixture = fix; > + const guchar helloworld[] = "Hello World!\n"; Do we have to use guchar, or can we just stick with 'char' or 'unsigned char'? > + > + /* read (check implicit flush) */ Here, the term implicit is okay; while the previous patch is adding an explicit flush at the low level, it is doing so in order to avoid having to make clients need an explicit flush operation (clients can rely on an implicit flush). > + cmd = g_strdup_printf("{'execute': 'guest-file-read'," > + " 'arguments': { 'handle': %" PRId64 "} }", > + id); > + ret = qmp_fd(fixture->fd, cmd); > + val = qdict_get_qdict(ret, "return"); > + count = qdict_get_int(val, "count"); > + eof = qdict_get_bool(val, "eof"); > + b64 = qdict_get_str(val, "buf-b64"); > + g_assert_cmpint(count, ==, 0); > + g_assert(eof); > + g_assert_cmpstr(b64, ==, ""); > + QDECREF(ret); > + g_free(cmd); > + > + /* seek to 0*/ Space before */ > + cmd = g_strdup_printf("{'execute': 'guest-file-seek'," > + " 'arguments': { 'handle': %" PRId64 ", " > + " 'offset': %d, 'whence': %d } }", > + id, 0, SEEK_SET); EWWWW. We seriously released this interface as taking an integer for whence? SEEK_SET is not required to be the same value on every platform. Which is a severe problem if the guest and the host are on different OS with different choices of values for the constants (if SEEK_CUR on my host is 1, but 1 maps to SEEK_END on my guest OS, what behavior am I going to get?). It would be worth a patch to qga to document the actual integer values that we have hard-coded (0 for set, 1 for cur, 2 for end; even if that differs from the guest's local definition of the SEEK_ constants), and/or to fix the interface to take symbolic names rather than integers for the whence argument. Our whole guest-file-* API is lousy.
Hi ----- Original Message ----- > On 11/24/2015 09:34 AM, marcandre.lureau@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > This test exhibits a POSIX behaviour regarding switching between write > > and read. It's undefined result if the application doesn't ensure a > > flush between the two operations (with glibc, the flush can be implicit > > when the buffer size is relatively small). The previous commit fixes > > this test. > > > > Related to: > > https://bugzilla.redhat.com/show_bug.cgi?id=1210246 > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > tests/test-qga.c | 91 > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 91 insertions(+) > > > > diff --git a/tests/test-qga.c b/tests/test-qga.c > > index 6473846..6b6963e 100644 > > --- a/tests/test-qga.c > > +++ b/tests/test-qga.c > > @@ -496,6 +496,96 @@ static void test_qga_file_ops(gconstpointer fix) > > g_free(cmd); > > } > > > > +static void test_qga_file_write_read(gconstpointer fix) > > +{ > > + const TestFixture *fixture = fix; > > + const guchar helloworld[] = "Hello World!\n"; > > Do we have to use guchar, or can we just stick with 'char' or 'unsigned > char'? We can switch to "unsigned char" > > > > + > > + /* read (check implicit flush) */ > > Here, the term implicit is okay; while the previous patch is adding an > explicit flush at the low level, it is doing so in order to avoid having > to make clients need an explicit flush operation (clients can rely on an > implicit flush). ok > > > + cmd = g_strdup_printf("{'execute': 'guest-file-read'," > > + " 'arguments': { 'handle': %" PRId64 "} }", > > + id); > > + ret = qmp_fd(fixture->fd, cmd); > > + val = qdict_get_qdict(ret, "return"); > > + count = qdict_get_int(val, "count"); > > + eof = qdict_get_bool(val, "eof"); > > + b64 = qdict_get_str(val, "buf-b64"); > > + g_assert_cmpint(count, ==, 0); > > + g_assert(eof); > > + g_assert_cmpstr(b64, ==, ""); > > + QDECREF(ret); > > + g_free(cmd); > > + > > + /* seek to 0*/ > > Space before */ fixed > > > + cmd = g_strdup_printf("{'execute': 'guest-file-seek'," > > + " 'arguments': { 'handle': %" PRId64 ", " > > + " 'offset': %d, 'whence': %d } }", > > + id, 0, SEEK_SET); > > EWWWW. We seriously released this interface as taking an integer for > whence? SEEK_SET is not required to be the same value on every > platform. Which is a severe problem if the guest and the host are on > different OS with different choices of values for the constants (if > SEEK_CUR on my host is 1, but 1 maps to SEEK_END on my guest OS, what > behavior am I going to get?). > > It would be worth a patch to qga to document the actual integer values > that we have hard-coded (0 for set, 1 for cur, 2 for end; even if that > differs from the guest's local definition of the SEEK_ constants), > and/or to fix the interface to take symbolic names rather than integers > for the whence argument. > > Our whole guest-file-* API is lousy. Are you going to send a patch for this? (hopefully, we can expose a "real" filesystem protocol in the near future)
On 11/24/2015 10:58 AM, Marc-André Lureau wrote: >>> + cmd = g_strdup_printf("{'execute': 'guest-file-seek'," >>> + " 'arguments': { 'handle': %" PRId64 ", " >>> + " 'offset': %d, 'whence': %d } }", >>> + id, 0, SEEK_SET); >> >> EWWWW. We seriously released this interface as taking an integer for >> whence? SEEK_SET is not required to be the same value on every >> platform. Which is a severe problem if the guest and the host are on >> different OS with different choices of values for the constants (if >> SEEK_CUR on my host is 1, but 1 maps to SEEK_END on my guest OS, what >> behavior am I going to get?). >> >> It would be worth a patch to qga to document the actual integer values >> that we have hard-coded (0 for set, 1 for cur, 2 for end; even if that >> differs from the guest's local definition of the SEEK_ constants), >> and/or to fix the interface to take symbolic names rather than integers >> for the whence argument. >> >> Our whole guest-file-* API is lousy. > > Are you going to send a patch for this? Sure, now that you've asked. For 2.5, it will just be documentation and mapping integers to the correct constants (any magic of using a qapi alternate type to support symbolic names would be 2.6 territory).
diff --git a/tests/test-qga.c b/tests/test-qga.c index 6473846..6b6963e 100644 --- a/tests/test-qga.c +++ b/tests/test-qga.c @@ -496,6 +496,96 @@ static void test_qga_file_ops(gconstpointer fix) g_free(cmd); } +static void test_qga_file_write_read(gconstpointer fix) +{ + const TestFixture *fixture = fix; + const guchar helloworld[] = "Hello World!\n"; + const char *b64; + gchar *cmd, *enc; + QDict *ret, *val; + int64_t id, eof; + gsize count; + + /* open */ + ret = qmp_fd(fixture->fd, "{'execute': 'guest-file-open'," + " 'arguments': { 'path': 'foo', 'mode': 'w+' } }"); + g_assert_nonnull(ret); + qmp_assert_no_error(ret); + id = qdict_get_int(ret, "return"); + QDECREF(ret); + + enc = g_base64_encode(helloworld, sizeof(helloworld)); + /* write */ + cmd = g_strdup_printf("{'execute': 'guest-file-write'," + " 'arguments': { 'handle': %" PRId64 "," + " 'buf-b64': '%s' } }", id, enc); + ret = qmp_fd(fixture->fd, cmd); + g_assert_nonnull(ret); + qmp_assert_no_error(ret); + + val = qdict_get_qdict(ret, "return"); + count = qdict_get_int(val, "count"); + eof = qdict_get_bool(val, "eof"); + g_assert_cmpint(count, ==, sizeof(helloworld)); + g_assert_cmpint(eof, ==, 0); + QDECREF(ret); + g_free(cmd); + + /* read (check implicit flush) */ + cmd = g_strdup_printf("{'execute': 'guest-file-read'," + " 'arguments': { 'handle': %" PRId64 "} }", + id); + ret = qmp_fd(fixture->fd, cmd); + val = qdict_get_qdict(ret, "return"); + count = qdict_get_int(val, "count"); + eof = qdict_get_bool(val, "eof"); + b64 = qdict_get_str(val, "buf-b64"); + g_assert_cmpint(count, ==, 0); + g_assert(eof); + g_assert_cmpstr(b64, ==, ""); + QDECREF(ret); + g_free(cmd); + + /* seek to 0*/ + cmd = g_strdup_printf("{'execute': 'guest-file-seek'," + " 'arguments': { 'handle': %" PRId64 ", " + " 'offset': %d, 'whence': %d } }", + id, 0, SEEK_SET); + ret = qmp_fd(fixture->fd, cmd); + qmp_assert_no_error(ret); + val = qdict_get_qdict(ret, "return"); + count = qdict_get_int(val, "position"); + eof = qdict_get_bool(val, "eof"); + g_assert_cmpint(count, ==, 0); + g_assert(!eof); + QDECREF(ret); + g_free(cmd); + + /* read */ + cmd = g_strdup_printf("{'execute': 'guest-file-read'," + " 'arguments': { 'handle': %" PRId64 "} }", + id); + ret = qmp_fd(fixture->fd, cmd); + val = qdict_get_qdict(ret, "return"); + count = qdict_get_int(val, "count"); + eof = qdict_get_bool(val, "eof"); + b64 = qdict_get_str(val, "buf-b64"); + g_assert_cmpint(count, ==, sizeof(helloworld)); + g_assert(eof); + g_assert_cmpstr(b64, ==, enc); + QDECREF(ret); + g_free(cmd); + g_free(enc); + + /* close */ + cmd = g_strdup_printf("{'execute': 'guest-file-close'," + " 'arguments': {'handle': %" PRId64 "} }", + id); + ret = qmp_fd(fixture->fd, cmd); + QDECREF(ret); + g_free(cmd); +} + static void test_qga_get_time(gconstpointer fix) { const TestFixture *fixture = fix; @@ -762,6 +852,7 @@ int main(int argc, char **argv) g_test_add_data_func("/qga/get-memory-blocks", &fix, test_qga_get_memory_blocks); g_test_add_data_func("/qga/file-ops", &fix, test_qga_file_ops); + g_test_add_data_func("/qga/file-write-read", &fix, test_qga_file_write_read); g_test_add_data_func("/qga/get-time", &fix, test_qga_get_time); g_test_add_data_func("/qga/invalid-cmd", &fix, test_qga_invalid_cmd); g_test_add_data_func("/qga/fsfreeze-status", &fix,