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

[Eric Blake]

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'?


> +
> +    /* 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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature