Re: [Qemu-devel] [PATCH v3 1/2] qga: flush explicitly when needed
On 11/25/2015 05:59 AM, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau
>
> 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
>
> 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
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 1/2] qga: flush explicitly when needed
On 11/25/15 13:59, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau
>
> 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 @@ GuestF
[Qemu-devel] [PATCH v3 1/2] qga: flush explicitly when needed
From: Marc-André Lureau
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
