From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34661) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1LwG-0006pS-0x for qemu-devel@nongnu.org; Tue, 24 Nov 2015 17:19:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a1LwB-0007oP-Rw for qemu-devel@nongnu.org; Tue, 24 Nov 2015 17:19:52 -0500 Received: from e36.co.us.ibm.com ([32.97.110.154]:47871) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1LwB-0007o4-LU for qemu-devel@nongnu.org; Tue, 24 Nov 2015 17:19:47 -0500 Received: from localhost by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 24 Nov 2015 15:19:42 -0700 Received: from b03cxnp08027.gho.boulder.ibm.com (b03cxnp08027.gho.boulder.ibm.com [9.17.130.19]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 2EBF019D8040 for ; Tue, 24 Nov 2015 15:07:47 -0700 (MST) Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by b03cxnp08027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id tAOMJetF30998772 for ; Tue, 24 Nov 2015 15:19:40 -0700 Received: from d03av01.boulder.ibm.com (localhost [127.0.0.1]) by d03av01.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id tAOMJd8t010828 for ; Tue, 24 Nov 2015 15:19:39 -0700 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth References: <1448388281-18691-1-git-send-email-marcandre.lureau@redhat.com> <1448388281-18691-2-git-send-email-marcandre.lureau@redhat.com> <5654CEEF.1040102@redhat.com> In-Reply-To: <5654CEEF.1040102@redhat.com> Message-ID: <20151124221929.6284.44169@loki> Date: Tue, 24 Nov 2015 16:19:29 -0600 Subject: Re: [Qemu-devel] [PATCH v2 1/2] qga: flush explicitly when needed List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , marcandre.lureau@redhat.com, qemu-devel@nongnu.org Quoting Eric Blake (2015-11-24 14:56:15) > On 11/24/2015 11:04 AM, marcandre.lureau@redhat.com wrote: > > From: Marc-Andr=C3=A9 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=3D1210246 > > = > > Signed-off-by: Marc-Andr=C3=A9 Lureau > > --- > > 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 =3D gfh->fh; > > + > > + /* explicitly flush when switching from writing to reading */ > > + if (gfh->writing) { > > + int ret =3D fflush(fh); > > + if (ret =3D=3D EOF) { > > + error_setg_errno(errp, errno, "failed to flush file"); > > + return NULL; > > + } > > + gfh->writing =3D false; > > + } > > + > > buf =3D g_malloc0(count+1); > > read_count =3D fread(buf, 1, count, fh); > > if (ferror(fh)) { > > @@ -496,6 +508,16 @@ GuestFileWrite *qmp_guest_file_write(int64_t handl= e, const char *buf_b64, > > } > > = > > fh =3D gfh->fh; > > + > > + if (!gfh->writing) { > > + int ret =3D fseek(fh, 0, SEEK_CUR); > > + if (ret =3D=3D -1) { > > + error_setg_errno(errp, errno, "failed to seek file"); > > + return NULL; > > + } > > + gfh->writing =3D 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 =3D 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 >=20