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

[Qemu-devel] [PATCH v2 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.

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 | 22 ++++++++++++
 tests/test-qga.c     | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 115 insertions(+), 2 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH v2 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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qga/commands-posix.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 0ebd473..d0228ce 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -219,6 +219,7 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
 typedef struct GuestFileHandle {
     uint64_t id;
     FILE *fh;
+    bool writing;
     QTAILQ_ENTRY(GuestFileHandle) next;
 } GuestFileHandle;
 
@@ -460,6 +461,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->writing) {
+        int ret = fflush(fh);
+        if (ret == EOF) {
+            error_setg_errno(errp, errno, "failed to flush file");
+            return NULL;
+        }
+        gfh->writing = false;
+    }
+
     buf = g_malloc0(count+1);
     read_count = fread(buf, 1, count, fh);
     if (ferror(fh)) {
@@ -496,6 +508,16 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
     }
 
     fh = gfh->fh;
+
+    if (!gfh->writing) {
+        int ret = fseek(fh, 0, SEEK_CUR);
+        if (ret == -1) {
+            error_setg_errno(errp, errno, "failed to seek file");
+            return NULL;
+        }
+        gfh->writing = true;
+    }
+
     buf = g_base64_decode(buf_b64, &buf_len);
 
     if (!has_count) {
-- 
2.5.0

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

[Eric Blake]

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

On 11/24/2015 11:04 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
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qga/commands-posix.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 0ebd473..d0228ce 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -219,6 +219,7 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
>  typedef struct GuestFileHandle {
>      uint64_t id;
>      FILE *fh;
> +    bool writing;
>      QTAILQ_ENTRY(GuestFileHandle) next;
>  } GuestFileHandle;
>  
> @@ -460,6 +461,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->writing) {
> +        int ret = fflush(fh);
> +        if (ret == EOF) {
> +            error_setg_errno(errp, errno, "failed to flush file");
> +            return NULL;
> +        }
> +        gfh->writing = false;
> +    }
> +
>      buf = g_malloc0(count+1);
>      read_count = fread(buf, 1, count, fh);
>      if (ferror(fh)) {
> @@ -496,6 +508,16 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
>      }
>  
>      fh = gfh->fh;
> +
> +    if (!gfh->writing) {
> +        int ret = fseek(fh, 0, SEEK_CUR);
> +        if (ret == -1) {
> +            error_setg_errno(errp, errno, "failed to seek file");
> +            return NULL;
> +        }
> +        gfh->writing = true;
> +    }

Hmm.  This always attempts fseek() on the first write() to a file, even
if the file is not also open for read.  While guest-file-open is most
likely used on regular files (and therefore seekable), I'm worried that
we might have a client that is attempting to use it on terminal files or
other non-seekable file names.  Since the fseek() on first write is
unconditional, that means we would now fail to let a user write to such
a file, even if they could previously do so.  Should we add more logic
to only do the fseek() after a previous write (as in a tri-state
variable of untouched, last written, last read), so that we aren't
breaking one-pass usage of non-seekable files?

-- 
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 v2 1/2] qga: flush explicitly when needed

[Michael Roth]

Quoting Eric Blake (2015-11-24 14:56:15)
> On 11/24/2015 11:04 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
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  qga/commands-posix.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 0ebd473..d0228ce 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -219,6 +219,7 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
> >  typedef struct GuestFileHandle {
> >      uint64_t id;
> >      FILE *fh;
> > +    bool writing;
> >      QTAILQ_ENTRY(GuestFileHandle) next;
> >  } GuestFileHandle;
> >  
> > @@ -460,6 +461,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->writing) {
> > +        int ret = fflush(fh);
> > +        if (ret == EOF) {
> > +            error_setg_errno(errp, errno, "failed to flush file");
> > +            return NULL;
> > +        }
> > +        gfh->writing = false;
> > +    }
> > +
> >      buf = g_malloc0(count+1);
> >      read_count = fread(buf, 1, count, fh);
> >      if (ferror(fh)) {
> > @@ -496,6 +508,16 @@ GuestFileWrite *qmp_guest_file_write(int64_t handle, const char *buf_b64,
> >      }
> >  
> >      fh = gfh->fh;
> > +
> > +    if (!gfh->writing) {
> > +        int ret = fseek(fh, 0, SEEK_CUR);
> > +        if (ret == -1) {
> > +            error_setg_errno(errp, errno, "failed to seek file");
> > +            return NULL;
> > +        }
> > +        gfh->writing = true;
> > +    }
> 
> Hmm.  This always attempts fseek() on the first write() to a file, even
> if the file is not also open for read.  While guest-file-open is most
> likely used on regular files (and therefore seekable), I'm worried that
> we might have a client that is attempting to use it on terminal files or
> other non-seekable file names.  Since the fseek() on first write is
> unconditional, that means we would now fail to let a user write to such
> a file, even if they could previously do so.  Should we add more logic
> to only do the fseek() after a previous write (as in a tri-state
> variable of untouched, last written, last read), so that we aren't
> breaking one-pass usage of non-seekable files?

I think that would be prudent. !gfh->writing doesn't imply gfh->reading,
and failed calls to qmp_guest_file_read() should probably not set
gfh->writing to true either.

Maybe something like:

gfh->rw_state = RW_STATE_NEW | RW_STATE_READING | RW_STATE_WRITING, skip the
fseek()/fflush() on RW_NEW, and only move to RW_STATE_READING/WRITING when
fread()/fwrite(), respectively, actually succeed?

I guess that still leaves the possibility of writes failing after reads
on non-seekable files. Are such files always directional? Otherwise that
means there are cases where it's impossible to implement the
requirements of the spec.

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

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

[Eric Blake]

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

On 11/24/2015 03:19 PM, Michael Roth wrote:

>> Hmm.  This always attempts fseek() on the first write() to a file, even
>> if the file is not also open for read.  While guest-file-open is most
>> likely used on regular files (and therefore seekable), I'm worried that
>> we might have a client that is attempting to use it on terminal files or
>> other non-seekable file names.  Since the fseek() on first write is
>> unconditional, that means we would now fail to let a user write to such
>> a file, even if they could previously do so.  Should we add more logic
>> to only do the fseek() after a previous write (as in a tri-state
>> variable of untouched, last written, last read), so that we aren't
>> breaking one-pass usage of non-seekable files?
> 
> I think that would be prudent. !gfh->writing doesn't imply gfh->reading,
> and failed calls to qmp_guest_file_read() should probably not set
> gfh->writing to true either.
> 
> Maybe something like:
> 
> gfh->rw_state = RW_STATE_NEW | RW_STATE_READING | RW_STATE_WRITING, skip the
> fseek()/fflush() on RW_NEW, and only move to RW_STATE_READING/WRITING when
> fread()/fwrite(), respectively, actually succeed?

Additionally, if guest-file-seek succeeds, reset state to RW_STATE_NEW
(any user-triggered seek satisfies the requirement so that we don't need
our implicit seek).

> 
> I guess that still leaves the possibility of writes failing after reads
> on non-seekable files. Are such files always directional? Otherwise that
> means there are cases where it's impossible to implement the
> requirements of the spec.

Remember, /dev/tty is commonly opened for both reading and writing - but
via separate fds (you read from fd 0, write to fd 1) - and it is the
console or terminal emulator that sets up the initial things (you seldom
see scripts directly attempting a bi-directional 'exec <> /dev/tty'.  I
think we'll be okay on that front.  Or if we're paranoid, then have
guest-file-seek inspect errno - if seek failed due to ESPIPE, then the
file is non-seekable, stdio shouldn't be buffering anyways, and we can
just blindly force RW_STATE_NEW in that case (the real reason for
fflush/fseek between read/write transitions is for seekable files).

-- 
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 v2 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 v2 2/2] tests: add file-write-read test

[Eric Blake]

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

On 11/24/2015 11:04 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(-)

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

If my patch goes in first, you'll want this to be s/SEEK_SET/0/ to
match.  (If yours goes in first, I'll spin a v2 of my patch to cover all
sites in the testsuite that need munging).

https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg05454.html

With that figured out,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 for-2.5 v2 0/2] qga: flush explicitly when needed

[Eric Blake]

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

On 11/24/2015 11:04 AM, marcandre.lureau@redhat.com wrote:
> 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.
> 
> v1->v2:
> - replace guchar with unsigned char
> - fix implicitly/explictly
> - comment space fix

Counts as a bug fix, so I'm suggesting that this series goes in 2.5;
although the behavior itself has been broken for a longer time so it is
not critical if it stalls until 2.6.

> 
> Marc-André Lureau (2):
>   qga: flush explicitly when needed
>   tests: add file-write-read test
> 
>  qga/commands-posix.c | 22 ++++++++++++
>  tests/test-qga.c     | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 115 insertions(+), 2 deletions(-)
> 

-- 
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 --]