From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34520) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFnGL-0004YG-7I for qemu-devel@nongnu.org; Tue, 18 Feb 2014 11:11:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFnGH-0005ST-08 for qemu-devel@nongnu.org; Tue, 18 Feb 2014 11:11:13 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60286) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFnGG-0005SK-O2 for qemu-devel@nongnu.org; Tue, 18 Feb 2014 11:11:08 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s1IGB6MX009339 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 18 Feb 2014 11:11:06 -0500 Message-ID: <53038619.6090209@redhat.com> Date: Tue, 18 Feb 2014 09:11:05 -0700 From: Eric Blake MIME-Version: 1.0 References: <1392738996-28867-1-git-send-email-quintela@redhat.com> In-Reply-To: <1392738996-28867-1-git-send-email-quintela@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="6x6EbnUOBr8rgDdxWa0DBj1cPnQtdMmvh" Subject: Re: [Qemu-devel] [PATCH v3] qemu_file: use fwrite() correctly List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela , qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --6x6EbnUOBr8rgDdxWa0DBj1cPnQtdMmvh Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 02/18/2014 08:56 AM, Juan Quintela wrote: > fwrite() returns the number of items written. But when there is one > error, it can return a short write. >=20 > In the particular bug that I was tracking, I did a migration to a > read-only filesystem. And it was able to finish the migration > correctly. fwrite() never returned a negative error code, nor zero, > always 4096. (migration writes chunks of about 14000 bytes). And it > was able to "complete" the migration with success (yes, reading the > file was a bit more difficult). >=20 > To add insult to injury, if your amount of memory was big enough (12GB > on my case), it overwrote some important structure, and from them, > malloc failed. This check makes the problem go away. >=20 > Signed-off-by: Juan Quintela > --- >=20 > v3: fwrite don't set errno in all cases. Huh? What cases would those be? http://pubs.opengroup.org/onlinepubs/9699919799/functions/fwrite.html is quite clear - POSIX requires that the only time fwrite() can return a smaller number of elements written is if it encountered a write failure as if by failed fputc(), and since fputc() is required to set errno on write failure, likewise, fwrite() is guaranteed to set errno on short writes. >> The fwrite() function shall return the number of elements successfully= written, which may be less than nitems if a write error is encountered. = If size or nitems is 0, fwrite() shall return 0 and the state of the stre= am remains unchanged. Otherwise, if a write error occurs, the error indic= ator for the stream shall be set, [CX] [Option Start] and errno shall be= set to indicate the error. [Option End] > +++ b/qemu-file.c > @@ -100,7 +100,17 @@ static int stdio_put_buffer(void *opaque, const ui= nt8_t *buf, int64_t pos, > int size) > { > QEMUFileStdio *s =3D opaque; > - return fwrite(buf, 1, size, s->stdio_file); > + int res; > + > + res =3D fwrite(buf, 1, size, s->stdio_file); > + > + if (res !=3D size) { > + if (errno) { > + return -errno; Even if your claim that fwrite doesn't always set errno on short writes were true, your patch here is wrong. Remember, POSIX states that you CANNOT assume the value of errno is reliable unless a function returned an error that indicates it was set, or for certain functions (like strtol) that are required to leave errno unchanged on success. POSIX is also clear that no library function (including fwrite) may explicitly set errno to 0 - they can only preserve its former value, or set it to a non-zero value. Furthermore, for functions which are not required to leave errno unchanged on success, it is perfectly acceptable for those functions (including fwrite) to clobber errno to some other value, even when returning success, all because you cannot rely on the errno value except when the function returned explicit failure. So, since fwrite is not required to keep errno unchanged on success, if you were right that there really is a case where it is not required to set errno on failure, then this patch has a problem - you do not pre-set errno=3D0, therefore, you cannot guarantee that you are not leaking some prior errno value from some prior function call. NAK to this version; v2 was better. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --6x6EbnUOBr8rgDdxWa0DBj1cPnQtdMmvh Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJTA4YZAAoJEKeha0olJ0NqCw0H/Ahe701OZvU1/rlLga1PLbpD 62WHgUJ4J9bKvWiTUbo6HGFj6DS8f4lM9IOuk0XfdgTOuwg08mko2cQ4C/EaZ7lp WTBBm8ixm9xk/cTb4Txme8P4phWyqxUytFYjFZMcCNlpZA3KX0q1UiirZ6fSMG0q OmqcMOeFIiaZOwqaMd5Br6b7Vzpr9J7v7g0tTj73BHCEwK4Rpvz4dkc052RiIqp3 k2xRAYAmQ1nPaH/InmxVg/dQ66huFBPJZng1qITW7zLyUlpp9xpXcKPaL3E6pK/Q bJz5Qe5/iLT0+c1mesFIzlm04OMso359FlUoSjbvApWIm1/jhrFYYvLaHgUWYcs= =hF6P -----END PGP SIGNATURE----- --6x6EbnUOBr8rgDdxWa0DBj1cPnQtdMmvh--