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

[Qemu-devel] [PATCH] 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>
---
 qemu-file.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

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

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

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

[Paolo Bonzini]

Il 11/02/2014 22:56, Juan Quintela ha scritto:
> 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>
> ---
>  qemu-file.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> 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 */

Can you return -errno here?  No need for a fake value.

Paolo

> +    }
> +    return res;
>  }
>
>  static int stdio_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
>

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

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 11/02/2014 22:56, Juan Quintela ha scritto:
>> 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>
>> ---
>>  qemu-file.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> 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 */
>
> Can you return -errno here?  No need for a fake value.

Sending v2.  On the 1st round here, it returns -EPIPE on errno, exactly
the error that has happened.

Thanks.

> Paolo
>
>> +    }
>> +    return res;
>>  }
>>
>>  static int stdio_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
>>