[Qemu-devel] [PATCH v3] qemu_file: use fwrite() correctly

[Qemu-devel] [PATCH v3] qemu_file: use fwrite() correctly

[Juan Quintela]

fwrite() returns the number of items written.  But when there is one
error, it can return a short write.

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).

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.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---

v3: fwrite don't set errno in all cases.

v2: a.k.a Paolo was right

    On the first call to fwrite() it returns 0, and errno is setup to
    EPIPE, exactly what we wanted.

    Once here, improve the commit message.


 qemu-file.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/qemu-file.c b/qemu-file.c
index 9473b67..4d5465e 100644
--- a/qemu-file.c
+++ b/qemu-file.c
@@ -100,7 +100,17 @@ static int stdio_put_buffer(void *opaque, const uint8_t *buf, int64_t pos,
                             int size)
 {
     QEMUFileStdio *s = opaque;
-    return fwrite(buf, 1, size, s->stdio_file);
+    int res;
+
+    res = fwrite(buf, 1, size, s->stdio_file);
+
+    if (res != size) {
+        if (errno) {
+            return -errno;
+        }
+        return -EIO;	/* fake errno value */
+    }
+    return res;
 }

 static int stdio_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v3] qemu_file: use fwrite() correctly

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3436 bytes --]

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.
> 
> 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).
> 
> 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.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
> 
> 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 stream remains unchanged. Otherwise, if a write error occurs, the error indicator 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 uint8_t *buf, int64_t pos,
>                              int size)
>  {
>      QEMUFileStdio *s = opaque;
> -    return fwrite(buf, 1, size, s->stdio_file);
> +    int res;
> +
> +    res = fwrite(buf, 1, size, s->stdio_file);
> +
> +    if (res != 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=0, 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.
-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]