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

[Qemu-devel] [PATCH v2] 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, the 1st
time it returns 0, after that it returns 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).  On the 1st fwrite() for the read-only filesystem,
it returns an errno of -EPIPE, that is exactly what has failed.

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

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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/qemu-file.c b/qemu-file.c
index 9473b67..e5ec798 100644
--- a/qemu-file.c
+++ b/qemu-file.c
@@ -100,7 +100,14 @@ 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) {
+        return -errno;
+    }
+    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 v2] qemu_file: use fwrite() correctly

[Eric Blake]

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

On 02/11/2014 05:00 PM, 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, the 1st
> time it returns 0, after that it returns 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).  On the 1st fwrite() for the read-only filesystem,
> it returns an errno of -EPIPE, that is exactly what has failed.
> 
> 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>
> ---
> 
> 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.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]

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

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> 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, the 1st
> time it returns 0, after that it returns 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).  On the 1st fwrite() for the read-only filesystem,
> it returns an errno of -EPIPE, that is exactly what has failed.
>
> 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>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Markus Armbruster]

Juan Quintela <quintela@redhat.com> writes:

> 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, the 1st
> time it returns 0, after that it returns 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).  On the 1st fwrite() for the read-only filesystem,
> it returns an errno of -EPIPE, that is exactly what has failed.
>
> 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>
> ---
>
> 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 | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-file.c b/qemu-file.c
> index 9473b67..e5ec798 100644
> --- a/qemu-file.c
> +++ b/qemu-file.c
> @@ -100,7 +100,14 @@ 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) {
> +        return -errno;
> +    }
> +    return res;
>  }
>
>  static int stdio_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)

This is what got an R-by from Eric and me.  And this is what Peter
pulled from Juan:

  diff --git a/qemu-file.c b/qemu-file.c
  index 9473b67..f074af1 100644
  --- a/qemu-file.c
  +++ b/qemu-file.c
  @@ -100,7 +100,14 @@ 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) {
  +        return -EIO;	/* fake errno value */
  +    }
  +    return res;
   }

Follow-up fix sent.