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

[Qemu-devel] [PATCH 1/2] qga: flush implicitely 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..3c86a4e 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;
+
+    /* implicitely 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

[Qemu-devel] [PATCH 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 | 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";
+    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 1/2] qga: flush implicitely when needed

[Eric Blake]

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

On 11/24/2015 09:34 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>

In the subject: s/implicitely/implicitly/ if you are fixing the typo, or
s/implicitely/explicitly/ if you are trying to make it match what the
patch actually does.

No 0/2 cover letter?  ALL multi-patch series should include a cover
letter, as it is easier on tooling to be able to base series-wide
conversations on the cover letter.

> 
> 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..3c86a4e 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;
> +
> +    /* implicitely flush when switching from writing to reading */

Again, s/implicitely/explicitly/

> +    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);

Seems a bit odd to use fflush() in one place and fseek() in the other,
but the net result is the same either way.

> +        if (ret == -1) {
> +            error_setg_errno(errp, errno, "failed to seek file");
> +            return NULL;
> +        }
> +        gfh->writing = true;
> +    }
> +

With typos fixed,
Reviewed-by: Eric Blake <eblake@redhat.com>

>      buf = g_base64_decode(buf_b64, &buf_len);
>  
>      if (!has_count) {
> 

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

[Eric Blake]

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

On 11/24/2015 09:34 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 | 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


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

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

[Marc-André Lureau]

Hi

----- Original Message -----
> On 11/24/2015 09:34 AM, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> In the subject: s/implicitely/implicitly/ if you are fixing the typo, or
> s/implicitely/explicitly/ if you are trying to make it match what the
> patch actually does.
> 

ok, I'll switch to explicitely (it depends on the point of view, I was commenting from the qga API user pov, but I get your point)
 
> No 0/2 cover letter?  ALL multi-patch series should include a cover
> letter, as it is easier on tooling to be able to base series-wide
> conversations on the cover letter.
> 

Ok, I didn't know. If I don't have much to say in cover letter, I usually drop it. I'll keep it then.

> > 
> > 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..3c86a4e 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;
> > +
> > +    /* implicitely flush when switching from writing to reading */
> 
> Again, s/implicitely/explicitly/
> 
> > +    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);
> 
> Seems a bit odd to use fflush() in one place and fseek() in the other,
> but the net result is the same either way.

"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."

so I tried to follow what the spec said.

> 
> > +        if (ret == -1) {
> > +            error_setg_errno(errp, errno, "failed to seek file");
> > +            return NULL;
> > +        }
> > +        gfh->writing = true;
> > +    }
> > +
> 
> With typos fixed,
> Reviewed-by: Eric Blake <eblake@redhat.com>

thanks

> 
> >      buf = g_base64_decode(buf_b64, &buf_len);
> >  
> >      if (!has_count) {
> > 
> 
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 
> 

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, 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 | 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)

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

[Eric Blake]

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

On 11/24/2015 10:58 AM, Marc-André Lureau wrote:

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

Sure, now that you've asked. For 2.5, it will just be documentation and
mapping integers to the correct constants (any magic of using a qapi
alternate type to support symbolic names would be 2.6 territory).

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

[Eric Blake]

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

On 11/24/2015 10:52 AM, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
>> On 11/24/2015 09:34 AM, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> In the subject: s/implicitely/implicitly/ if you are fixing the typo, or
>> s/implicitely/explicitly/ if you are trying to make it match what the
>> patch actually does.
>>
> 
> ok, I'll switch to explicitely (it depends on the point of view, I was commenting from the qga API user pov, but I get your point)

I was trying to point out not only the 2 points of view, but also the
typo (it's explicitly, not explicitely) :)

>>>      fh = gfh->fh;
>>> +
>>> +    if (!gfh->writing) {
>>> +        int ret = fseek(fh, 0, SEEK_CUR);
>>
>> Seems a bit odd to use fflush() in one place and fseek() in the other,
>> but the net result is the same either way.
> 
> "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."
> 
> so I tried to follow what the spec said.

POSIX currently specifies the behavior of fflush() on seekable input
files, but did not always do so; and it has been a source of bugs on
several libc implementations (it is still undefined to use fflush() on a
non-seekable file, but I don't know if anyone is using qga guest-file-*
on non-seekable files, at least in a situation where they are both
reading and writing to the same file handle).  So on further thought, I
actually prefer avoiding fflush() after input when possible, to avoid
confusing older libc, and as a result, your asymmetry is probably the
best choice after all.

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