[Qemu-devel] [PATCH v3 0/2] qga: flush explicitly when needed

[Qemu-devel] [PATCH v3 0/2] qga: flush explicitly when needed

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Without this change, a write() followed by a read() may lose the
previously written content, as shown in the following test.

v2->v3:
- use a RwState tristate enum
- reset the state on flush & seek

v1->v2:
- replace guchar with unsigned char
- fix implicitly/explictly
- comment space fix

Marc-André Lureau (2):
  qga: flush explicitly when needed
  tests: add file-write-read test

 qga/commands-posix.c | 37 ++++++++++++++++++++
 tests/test-qga.c     | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 130 insertions(+), 2 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH v3 1/2] qga: flush explicitly when needed

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

According to the specification:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html

"the application shall ensure that output is not directly followed by
input without an intervening call to fflush() or to a file positioning
function (fseek(), fsetpos(), or rewind()), and input is not directly
followed by output without an intervening call to a file positioning
function, unless the input operation encounters end-of-file."

Without this change, a write() followed by a read() may lose the
previously written content, as shown in the following test.

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1210246
---
 qga/commands-posix.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 0ebd473..cf1d7ec 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -216,9 +216,16 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
     }
 }
 
+typedef enum {
+    RW_STATE_NEW,
+    RW_STATE_READING,
+    RW_STATE_WRITING,
+} RwState;
+
 typedef struct GuestFileHandle {
     uint64_t id;
     FILE *fh;
+    RwState state;
     QTAILQ_ENTRY(GuestFileHandle) next;
 } GuestFileHandle;
 
@@ -460,6 +467,17 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
     }
 
     fh = gfh->fh;
+
+    /* explicitly flush when switching from writing to reading */
+    if (gfh->state == RW_STATE_WRITING) {
+        int ret = fflush(fh);
+        if (ret == EOF) {
+            error_setg_errno(errp, errno, "failed to flush file");
+            return NULL;
+        }
+        gfh->state = RW_STATE_NEW;
+    }
+
     buf = g_malloc0(count+1);
     read_count = fread(buf, 1, count, fh);
     if (ferror(fh)) {
@@ -473,6 +491,7 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
         if (read_count) {
             read_data->buf_b64 = g_base64_encode(buf, read_count);
         }
+        gfh->state = RW_STATE_READING;
     }
     g_free(buf);
     clearerr(fh);
@@ -496,6 +515,16 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
     }
 
     fh = gfh->fh;
+
+    if (gfh->state == RW_STATE_READING) {
+        int ret = fseek(fh, 0, SEEK_CUR);
+        if (ret == -1) {
+            error_setg_errno(errp, errno, "failed to seek file");
+            return NULL;
+        }
+        gfh->state = RW_STATE_NEW;
+    }
+
     buf = g_base64_decode(buf_b64, &buf_len);
 
     if (!has_count) {
@@ -515,6 +544,7 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
         write_data = g_new0(GuestFileWrite, 1);
         write_data->count = write_count;
         write_data->eof = feof(fh);
+        gfh->state = RW_STATE_WRITING;
     }
     g_free(buf);
     clearerr(fh);
@@ -538,10 +568,15 @@ struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
     ret = fseek(fh, offset, whence);
     if (ret == -1) {
         error_setg_errno(errp, errno, "failed to seek file");
+        if (errno == ESPIPE) {
+            /* file is non-seekable, stdio shouldn't be buffering anyways */
+            gfh->state = RW_STATE_NEW;
+        }
     } else {
         seek_data = g_new0(GuestFileSeek, 1);
         seek_data->position = ftell(fh);
         seek_data->eof = feof(fh);
+        gfh->state = RW_STATE_NEW;
     }
     clearerr(fh);
 
@@ -562,6 +597,8 @@ void qmp_guest_file_flush(int64_t handle, Error **errp)
     ret = fflush(fh);
     if (ret == EOF) {
         error_setg_errno(errp, errno, "failed to flush file");
+    } else {
+        gfh->state = RW_STATE_NEW;
     }
 }
 
-- 
2.5.0

Re: [Qemu-devel] [PATCH v3 1/2] qga: flush explicitly when needed

[Laszlo Ersek]

On 11/25/15 13:59, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> According to the specification:
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html
> 
> "the application shall ensure that output is not directly followed by
> input without an intervening call to fflush() or to a file positioning
> function (fseek(), fsetpos(), or rewind()), and input is not directly
> followed by output without an intervening call to a file positioning
> function, unless the input operation encounters end-of-file."
> 
> Without this change, a write() followed by a read() may lose the
> previously written content, as shown in the following test.

No. The very last paragraph is incorrect.

Specifically between read() and write(), the symptom is explicitly
forbidden by POSIX:

http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html

    After a write() to a regular file has successfully returned:

        * Any successful read() from each byte position in the file
          that was modified by that write shall return the data
          specified by the write() for that position until such byte
          positions are again modified.

However, because qga uses stdio streams, not file descriptors, the POSIX
quote that you included in the commit message *does* apply, my quote
doesn't, and the patch seems necessary.

I'm just saying that the last paragraph of the commit message contains
an untrue statement. If you modify it to say fwrite() and fread(), it
will be okay.

(

Independently, if you are interested: there is general language in POSIX
that concerns *file handles*. Not file descriptors, and also not stdio
streams -- file handles are an abstraction above both of those,
introduced specifically for the discussion of the interactions between
file descriptors and stdio streams.

2.5.1 Interaction of File Descriptors and Standard I/O Streams

http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_05_01

    [...] Either a file descriptor or a stream is called a "handle" on
    the open file description to which it refers; an open file
    description may have several handles.

    [...]

    The result of function calls involving any one handle (the "active
    handle") is defined elsewhere in this volume of POSIX.1-2008, but
    if two or more handles are used, and any one of them is a stream,
    the application shall ensure that their actions are coordinated as
    described below. If this is not done, the result is undefined.

It is very interesting in general, but admittedly not related to this
patch -- as far as I understand, in QGA there is only one handle to any
file description: the stdio stream.

)

I'm not volunteering to review the patch, hence I'm sorry about the
noise; I hope it wasn't worthless to point out the problem in the commit
message.

Thanks
Laszlo

> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1210246
> ---
>  qga/commands-posix.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 0ebd473..cf1d7ec 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -216,9 +216,16 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
>      }
>  }
>  
> +typedef enum {
> +    RW_STATE_NEW,
> +    RW_STATE_READING,
> +    RW_STATE_WRITING,
> +} RwState;
> +
>  typedef struct GuestFileHandle {
>      uint64_t id;
>      FILE *fh;
> +    RwState state;
>      QTAILQ_ENTRY(GuestFileHandle) next;
>  } GuestFileHandle;
>  
> @@ -460,6 +467,17 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
>      }
>  
>      fh = gfh->fh;
> +
> +    /* explicitly flush when switching from writing to reading */
> +    if (gfh->state == RW_STATE_WRITING) {
> +        int ret = fflush(fh);
> +        if (ret == EOF) {
> +            error_setg_errno(errp, errno, "failed to flush file");
> +            return NULL;
> +        }
> +        gfh->state = RW_STATE_NEW;
> +    }
> +
>      buf = g_malloc0(count+1);
>      read_count = fread(buf, 1, count, fh);
>      if (ferror(fh)) {
> @@ -473,6 +491,7 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
>          if (read_count) {
>              read_data->buf_b64 = g_base64_encode(buf, read_count);
>          }
> +        gfh->state = RW_STATE_READING;
>      }
>      g_free(buf);
>      clearerr(fh);
> @@ -496,6 +515,16 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
>      }
>  
>      fh = gfh->fh;
> +
> +    if (gfh->state == RW_STATE_READING) {
> +        int ret = fseek(fh, 0, SEEK_CUR);
> +        if (ret == -1) {
> +            error_setg_errno(errp, errno, "failed to seek file");
> +            return NULL;
> +        }
> +        gfh->state = RW_STATE_NEW;
> +    }
> +
>      buf = g_base64_decode(buf_b64, &buf_len);
>  
>      if (!has_count) {
> @@ -515,6 +544,7 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
>          write_data = g_new0(GuestFileWrite, 1);
>          write_data->count = write_count;
>          write_data->eof = feof(fh);
> +        gfh->state = RW_STATE_WRITING;
>      }
>      g_free(buf);
>      clearerr(fh);
> @@ -538,10 +568,15 @@ struct GuestFileSeek *qmp_guest_file_seek(int64_t handle, int64_t offset,
>      ret = fseek(fh, offset, whence);
>      if (ret == -1) {
>          error_setg_errno(errp, errno, "failed to seek file");
> +        if (errno == ESPIPE) {
> +            /* file is non-seekable, stdio shouldn't be buffering anyways */
> +            gfh->state = RW_STATE_NEW;
> +        }
>      } else {
>          seek_data = g_new0(GuestFileSeek, 1);
>          seek_data->position = ftell(fh);
>          seek_data->eof = feof(fh);
> +        gfh->state = RW_STATE_NEW;
>      }
>      clearerr(fh);
>  
> @@ -562,6 +597,8 @@ void qmp_guest_file_flush(int64_t handle, Error **errp)
>      ret = fflush(fh);
>      if (ret == EOF) {
>          error_setg_errno(errp, errno, "failed to flush file");
> +    } else {
> +        gfh->state = RW_STATE_NEW;
>      }
>  }
>  
> 

Re: [Qemu-devel] [PATCH v3 1/2] qga: flush explicitly when needed

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]

On 11/25/2015 05:59 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> According to the specification:
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html
> 
> "the application shall ensure that output is not directly followed by
> input without an intervening call to fflush() or to a file positioning
> function (fseek(), fsetpos(), or rewind()), and input is not directly
> followed by output without an intervening call to a file positioning
> function, unless the input operation encounters end-of-file."
> 
> Without this change, a write() followed by a read() may lose the
> previously written content, as shown in the following test.
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1210246
> ---
>  qga/commands-posix.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)

With Laszlo's suggested commit message improvements,
Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 0ebd473..cf1d7ec 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -216,9 +216,16 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
>      }
>  }
>  
> +typedef enum {
> +    RW_STATE_NEW,

Bikeshedding: NEW really only sounds right after open(), and doesn't
quite right after a flush; maybe CLEAN or WAITING would be a better name?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[marcandre.lureau]

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 | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 93 insertions(+), 2 deletions(-)

diff --git a/tests/test-qga.c b/tests/test-qga.c
index 6473846..3b99d9d 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -352,10 +352,10 @@ static void test_qga_network_get_interfaces(gconstpointer fix)
 static void test_qga_file_ops(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    const guchar helloworld[] = "Hello World!\n";
+    const unsigned char helloworld[] = "Hello World!\n";
     const char *b64;
     gchar *cmd, *path, *enc;
-    guchar *dec;
+    unsigned char *dec;
     QDict *ret, *val;
     int64_t id, eof;
     gsize count;
@@ -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 unsigned char 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,
-- 
2.5.0

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]

On 11/25/2015 05:59 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 | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 93 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> +    /* seek to 0 */
> +    cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
> +                          " 'arguments': { 'handle': %" PRId64 ", "
> +                          " 'offset': %d, 'whence': %d } }",
> +                          id, 0, SEEK_SET);

We still have a conflict between this series and my proposal to codify 0
rather than relying on platform-specific SEEK_SET; Markus had the
suggestion of using QGA_SET (or QGA_SEEK_SET).  Are we trying to get
both your series and my v2 patch into 2.5?  Knowing that will help me
decide whether my v2 should be rebased on top of your patches.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Michael Roth]

Quoting Eric Blake (2015-11-25 10:02:55)
> On 11/25/2015 05:59 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 | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 93 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> > +    /* seek to 0 */
> > +    cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
> > +                          " 'arguments': { 'handle': %" PRId64 ", "
> > +                          " 'offset': %d, 'whence': %d } }",
> > +                          id, 0, SEEK_SET);
> 
> We still have a conflict between this series and my proposal to codify 0
> rather than relying on platform-specific SEEK_SET; Markus had the
> suggestion of using QGA_SET (or QGA_SEEK_SET).  Are we trying to get
> both your series and my v2 patch into 2.5?  Knowing that will help me
> decide whether my v2 should be rebased on top of your patches.

I was planning on pulling in your patch on top of this for the next
2.5 pull, so rebasing on top of this series is probably best.

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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

On 11/25/2015 09:21 AM, Michael Roth wrote:

>>> +    /* seek to 0 */
>>> +    cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
>>> +                          " 'arguments': { 'handle': %" PRId64 ", "
>>> +                          " 'offset': %d, 'whence': %d } }",
>>> +                          id, 0, SEEK_SET);
>>
>> We still have a conflict between this series and my proposal to codify 0
>> rather than relying on platform-specific SEEK_SET; Markus had the
>> suggestion of using QGA_SET (or QGA_SEEK_SET).  Are we trying to get
>> both your series and my v2 patch into 2.5?  Knowing that will help me
>> decide whether my v2 should be rebased on top of your patches.
> 
> I was planning on pulling in your patch on top of this for the next
> 2.5 pull, so rebasing on top of this series is probably best.

Okay, will do, and will do quickly so I'm not holding up your pull request.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Michael Roth]

Quoting Eric Blake (2015-11-25 10:41:35)
> On 11/25/2015 09:21 AM, Michael Roth wrote:
> 
> >>> +    /* seek to 0 */
> >>> +    cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
> >>> +                          " 'arguments': { 'handle': %" PRId64 ", "
> >>> +                          " 'offset': %d, 'whence': %d } }",
> >>> +                          id, 0, SEEK_SET);
> >>
> >> We still have a conflict between this series and my proposal to codify 0
> >> rather than relying on platform-specific SEEK_SET; Markus had the
> >> suggestion of using QGA_SET (or QGA_SEEK_SET).  Are we trying to get
> >> both your series and my v2 patch into 2.5?  Knowing that will help me
> >> decide whether my v2 should be rebased on top of your patches.
> > 
> > I was planning on pulling in your patch on top of this for the next
> > 2.5 pull, so rebasing on top of this series is probably best.
> 
> Okay, will do, and will do quickly so I'm not holding up your pull request.

Thanks! FYI I now have this series applied here if you'd like to base
on that:

  https://github.com/mdroth/qemu/commits/qga

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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1389 bytes --]

On 11/25/2015 09:46 AM, Michael Roth wrote:
> Quoting Eric Blake (2015-11-25 10:41:35)
>> On 11/25/2015 09:21 AM, Michael Roth wrote:
>>
>>>>> +    /* seek to 0 */
>>>>> +    cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
>>>>> +                          " 'arguments': { 'handle': %" PRId64 ", "
>>>>> +                          " 'offset': %d, 'whence': %d } }",
>>>>> +                          id, 0, SEEK_SET);
>>>>
>>>> We still have a conflict between this series and my proposal to codify 0
>>>> rather than relying on platform-specific SEEK_SET; Markus had the
>>>> suggestion of using QGA_SET (or QGA_SEEK_SET).  Are we trying to get
>>>> both your series and my v2 patch into 2.5?  Knowing that will help me
>>>> decide whether my v2 should be rebased on top of your patches.
>>>
>>> I was planning on pulling in your patch on top of this for the next
>>> 2.5 pull, so rebasing on top of this series is probably best.
>>
>> Okay, will do, and will do quickly so I'm not holding up your pull request.
> 
> Thanks! FYI I now have this series applied here if you'd like to base
> on that:
> 
>   https://github.com/mdroth/qemu/commits/qga

On that branch, commit 7a38932 has two typos:
s/is commit msg (Lazlo)/in commit msg (Laszlo)/

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Michael Roth]

Quoting Eric Blake (2015-11-25 11:10:46)
> On 11/25/2015 09:46 AM, Michael Roth wrote:
> > Quoting Eric Blake (2015-11-25 10:41:35)
> >> On 11/25/2015 09:21 AM, Michael Roth wrote:
> >>
> >>>>> +    /* seek to 0 */
> >>>>> +    cmd = g_strdup_printf("{'execute': 'guest-file-seek',"
> >>>>> +                          " 'arguments': { 'handle': %" PRId64 ", "
> >>>>> +                          " 'offset': %d, 'whence': %d } }",
> >>>>> +                          id, 0, SEEK_SET);
> >>>>
> >>>> We still have a conflict between this series and my proposal to codify 0
> >>>> rather than relying on platform-specific SEEK_SET; Markus had the
> >>>> suggestion of using QGA_SET (or QGA_SEEK_SET).  Are we trying to get
> >>>> both your series and my v2 patch into 2.5?  Knowing that will help me
> >>>> decide whether my v2 should be rebased on top of your patches.
> >>>
> >>> I was planning on pulling in your patch on top of this for the next
> >>> 2.5 pull, so rebasing on top of this series is probably best.
> >>
> >> Okay, will do, and will do quickly so I'm not holding up your pull request.
> > 
> > Thanks! FYI I now have this series applied here if you'd like to base
> > on that:
> > 
> >   https://github.com/mdroth/qemu/commits/qga
> 
> On that branch, commit 7a38932 has two typos:
> s/is commit msg (Lazlo)/in commit msg (Laszlo)/

Thanks, fixed now.

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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 574 bytes --]

On 11/25/2015 10:14 AM, Michael Roth wrote:
>>> Thanks! FYI I now have this series applied here if you'd like to base
>>> on that:
>>>
>>>   https://github.com/mdroth/qemu/commits/qga
>>
>> On that branch, commit 7a38932 has two typos:
>> s/is commit msg (Lazlo)/in commit msg (Laszlo)/
> 
> Thanks, fixed now.

Except that in e8c9ea9, the message still references write()/read()
instead of the intended fwrite()/fread().  We'll get it eventually :)

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Michael Roth]

Quoting Eric Blake (2015-11-25 11:18:24)
> On 11/25/2015 10:14 AM, Michael Roth wrote:
> >>> Thanks! FYI I now have this series applied here if you'd like to base
> >>> on that:
> >>>
> >>>   https://github.com/mdroth/qemu/commits/qga
> >>
> >> On that branch, commit 7a38932 has two typos:
> >> s/is commit msg (Lazlo)/in commit msg (Laszlo)/
> > 
> > Thanks, fixed now.
> 
> Except that in e8c9ea9, the message still references write()/read()
> instead of the intended fwrite()/fread().  We'll get it eventually :)

Argh! Sorry, fixed now.

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

Re: [Qemu-devel] [PATCH v3 0/2] qga: flush explicitly when needed

[Michael Roth]

Quoting marcandre.lureau@redhat.com (2015-11-25 06:59:10)
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Without this change, a write() followed by a read() may lose the
> previously written content, as shown in the following test.
> 
> v2->v3:
> - use a RwState tristate enum
> - reset the state on flush & seek
> 
> v1->v2:
> - replace guchar with unsigned char
> - fix implicitly/explictly
> - comment space fix
> 
> Marc-André Lureau (2):
>   qga: flush explicitly when needed
>   tests: add file-write-read test

Thanks, applied with fix-ups suggested by Lazlo:

  https://github.com/mdroth/qemu/commits/qga

> 
>  qga/commands-posix.c | 37 ++++++++++++++++++++
>  tests/test-qga.c     | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 130 insertions(+), 2 deletions(-)
> 
> -- 
> 2.5.0
> 

Re: [Qemu-devel] [PATCH v3 0/2] qga: flush explicitly when needed

[Marc-André Lureau]

Hi

----- Original Message -----
> Quoting marcandre.lureau@redhat.com (2015-11-25 06:59:10)
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > Without this change, a write() followed by a read() may lose the
> > previously written content, as shown in the following test.
> > 
> > v2->v3:
> > - use a RwState tristate enum
> > - reset the state on flush & seek
> > 
> > v1->v2:
> > - replace guchar with unsigned char
> > - fix implicitly/explictly
> > - comment space fix
> > 
> > Marc-André Lureau (2):
> >   qga: flush explicitly when needed
> >   tests: add file-write-read test
> 
> Thanks, applied with fix-ups suggested by Lazlo:
> 
>   https://github.com/mdroth/qemu/commits/qga

thanks Michael!