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

[Laszlo Ersek]

On 11/25/15 13:59, address@hidden wrote:
> From: Marc-André Lureau <address@hidden>
> 
> 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;
>      }
>  }
>  
>