From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48980) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1Hln-0003HS-3n for qemu-devel@nongnu.org; Tue, 24 Nov 2015 12:52:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a1Hlj-0002Ry-MY for qemu-devel@nongnu.org; Tue, 24 Nov 2015 12:52:46 -0500 Received: from mx4-phx2.redhat.com ([209.132.183.25]:34531) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1Hlj-0002Ru-GV for qemu-devel@nongnu.org; Tue, 24 Nov 2015 12:52:43 -0500 Date: Tue, 24 Nov 2015 12:52:40 -0500 (EST) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <1705571877.16594173.1448387560812.JavaMail.zimbra@redhat.com> In-Reply-To: <56549B2E.6070205@redhat.com> References: <1448382858-28616-1-git-send-email-marcandre.lureau@redhat.com> <56549B2E.6070205@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/2] qga: flush implicitely when needed List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: marcandre lureau , qemu-devel@nongnu.org, Michael Roth Hi ----- Original Message ----- > On 11/24/2015 09:34 AM, marcandre.lureau@redhat.com wrote: > > From: Marc-Andr=C3=A9 Lureau >=20 > 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. >=20 ok, I'll switch to explicitely (it depends on the point of view, I was comm= enting from the qga API user pov, but I get your point) =20 > 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. >=20 Ok, I didn't know. If I don't have much to say in cover letter, I usually d= rop it. I'll keep it then. > >=20 > > According to the specification: > > http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html > >=20 > > "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." > >=20 > > Without this change, a write() followed by a read() may lose the > > previously written content, as shown in the following test. > >=20 > > Fixes: > > https://bugzilla.redhat.com/show_bug.cgi?id=3D1210246 > >=20 > > Signed-off-by: Marc-Andr=C3=A9 Lureau > > --- > > qga/commands-posix.c | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > >=20 > > 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; > > =20 > > @@ -460,6 +461,17 @@ struct GuestFileRead *qmp_guest_file_read(int64_t > > handle, bool has_count, > > } > > =20 > > fh =3D gfh->fh; > > + > > + /* implicitely flush when switching from writing to reading */ >=20 > Again, s/implicitely/explicitly/ >=20 > > + 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, > > } > > =20 > > fh =3D gfh->fh; > > + > > + if (!gfh->writing) { > > + int ret =3D fseek(fh, 0, SEEK_CUR); >=20 > 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 t= o a file positioning function, unless the input operation encounters end-of= -file." so I tried to follow what the spec said. >=20 > > + if (ret =3D=3D -1) { > > + error_setg_errno(errp, errno, "failed to seek file"); > > + return NULL; > > + } > > + gfh->writing =3D true; > > + } > > + >=20 > With typos fixed, > Reviewed-by: Eric Blake thanks >=20 > > buf =3D g_base64_decode(buf_b64, &buf_len); > > =20 > > if (!has_count) { > >=20 >=20 > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >=20 >=20