Hi
----- Original Message -----
> On 11/24/2015 09:34 AM, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lur...@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.lur...@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)
