Re: [Qemu-devel] [PATCH 2/2] tests: add file-write-read test

[Marc-André Lureau]

Hi

----- Original Message -----
> On 11/24/2015 09:34 AM, address@hidden wrote:
> > From: Marc-André Lureau <address@hidden>
> > 
> > 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 <address@hidden>
> > ---
> >  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)