[Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate

[Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate

[Max Reitz]

Fully preallocated truncation has a 50 % chance of working on images
files over file-posix.  It works if $SIZE % 4G < 2G, and it fails
otherwise.  To make things even more interesting, often you would not
even notice because qemu reported success even though it did nothing
(because after the successful lseek(), errno was still 0, so when the
file-posix driver tried to return a negative error code, it actually
reported success).

This issue is fixed by patch 1 in this series.  Thanks to Daniel for
reporting!


Max Reitz (2):
  block/file-posix: Fix fully preallocated truncate
  iotests: Test preallocated truncate of 2G image

 block/file-posix.c         |  5 +++--
 tests/qemu-iotests/106     | 24 ++++++++++++++++++++++++
 tests/qemu-iotests/106.out | 10 ++++++++++
 3 files changed, 37 insertions(+), 2 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate

[Max Reitz]

Storing the lseek() result in an int results in it overflowing when the
file is at least 2 GB big.  Then, we have a 50 % chance of the result
being "negative" and thus thinking an error occurred when actually
everything went just fine.

So we should use the correct type for storing the result: off_t.

Reported-by: Daniel P. Berrange <berrange@redhat.com>
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/file-posix.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index f1591c3849..90c25864a0 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
     case PREALLOC_MODE_FULL:
     {
         int64_t num = 0, left = offset - current_length;
+        off_t seek_result;
 
         /*
          * Knowing the final size from the beginning could allow the file
@@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
 
         buf = g_malloc0(65536);
 
-        result = lseek(fd, current_length, SEEK_SET);
-        if (result < 0) {
+        seek_result = lseek(fd, current_length, SEEK_SET);
+        if (seek_result < 0) {
             result = -errno;
             error_setg_errno(errp, -result,
                              "Failed to seek to the old end of file");
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
> Storing the lseek() result in an int results in it overflowing when the
> file is at least 2 GB big.  Then, we have a 50 % chance of the result
> being "negative" and thus thinking an error occurred when actually
> everything went just fine.
> 
> So we should use the correct type for storing the result: off_t.
> 
> Reported-by: Daniel P. Berrange <berrange@redhat.com>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/file-posix.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index f1591c3849..90c25864a0 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>      case PREALLOC_MODE_FULL:
>      {
>          int64_t num = 0, left = offset - current_length;
> +        off_t seek_result;
>  
>          /*
>           * Knowing the final size from the beginning could allow the file
> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>  
>          buf = g_malloc0(65536);
>  
> -        result = lseek(fd, current_length, SEEK_SET);
> -        if (result < 0) {
> +        seek_result = lseek(fd, current_length, SEEK_SET);
> +        if (seek_result < 0) {

off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
exact value (off_t)-1 indicates an error. So this needs to be

   if (seek_result == (off_t)-1) {
      ...
   }


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate

[Max Reitz]

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

On 2018-02-28 14:34, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
>> Storing the lseek() result in an int results in it overflowing when the
>> file is at least 2 GB big.  Then, we have a 50 % chance of the result
>> being "negative" and thus thinking an error occurred when actually
>> everything went just fine.
>>
>> So we should use the correct type for storing the result: off_t.
>>
>> Reported-by: Daniel P. Berrange <berrange@redhat.com>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/file-posix.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index f1591c3849..90c25864a0 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>>      case PREALLOC_MODE_FULL:
>>      {
>>          int64_t num = 0, left = offset - current_length;
>> +        off_t seek_result;
>>  
>>          /*
>>           * Knowing the final size from the beginning could allow the file
>> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>>  
>>          buf = g_malloc0(65536);
>>  
>> -        result = lseek(fd, current_length, SEEK_SET);
>> -        if (result < 0) {
>> +        seek_result = lseek(fd, current_length, SEEK_SET);
>> +        if (seek_result < 0) {
> 
> off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
> exact value (off_t)-1 indicates an error. So this needs to be
> 
>    if (seek_result == (off_t)-1) {
>       ...
>    }

Hmmm... On my system, it appears to be a long int[1].  And
find_allocation() does an off_t < 0 comparison already.  And
"man 0p sys_types.h" says "blkcnt_t and off_t shall be signed integer
types."

Max


[1]

#define do_stringify(x) #x



#define stringify(x) do_stringify(x)

int main(void)
{
    printf("%s\n", stringify(__OFF_T_TYPE));
}

Output: long int


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

Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 02:45:49PM +0100, Max Reitz wrote:
> On 2018-02-28 14:34, Daniel P. Berrangé wrote:
> > On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
> >> Storing the lseek() result in an int results in it overflowing when the
> >> file is at least 2 GB big.  Then, we have a 50 % chance of the result
> >> being "negative" and thus thinking an error occurred when actually
> >> everything went just fine.
> >>
> >> So we should use the correct type for storing the result: off_t.
> >>
> >> Reported-by: Daniel P. Berrange <berrange@redhat.com>
> >> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
> >> Cc: qemu-stable@nongnu.org
> >> Signed-off-by: Max Reitz <mreitz@redhat.com>
> >> ---
> >>  block/file-posix.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/block/file-posix.c b/block/file-posix.c
> >> index f1591c3849..90c25864a0 100644
> >> --- a/block/file-posix.c
> >> +++ b/block/file-posix.c
> >> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
> >>      case PREALLOC_MODE_FULL:
> >>      {
> >>          int64_t num = 0, left = offset - current_length;
> >> +        off_t seek_result;
> >>  
> >>          /*
> >>           * Knowing the final size from the beginning could allow the file
> >> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
> >>  
> >>          buf = g_malloc0(65536);
> >>  
> >> -        result = lseek(fd, current_length, SEEK_SET);
> >> -        if (result < 0) {
> >> +        seek_result = lseek(fd, current_length, SEEK_SET);
> >> +        if (seek_result < 0) {
> > 
> > off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
> > exact value (off_t)-1 indicates an error. So this needs to be
> > 
> >    if (seek_result == (off_t)-1) {
> >       ...
> >    }
> 
> Hmmm... On my system, it appears to be a long int[1].  And
> find_allocation() does an off_t < 0 comparison already.  And
> "man 0p sys_types.h" says "blkcnt_t and off_t shall be signed integer
> types."

Hmm, that's odd then - lseek man page explicitly said it must be cast,
which suggested to me it could be unsigned:

   RETURN VALUE
       Upon successful completion, lseek() returns the resulting offset  loca‐
       tion  as  measured  in bytes from the beginning of the file.  On error,
       the value (off_t) -1 is returned and  errno  is  set  to  indicate  the
       error.

CC'ing Eric for the "official" POSIX answer....

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate

[Max Reitz]

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

On 2018-02-28 14:53, Daniel P. Berrangé wrote:
> On Wed, Feb 28, 2018 at 02:45:49PM +0100, Max Reitz wrote:
>> On 2018-02-28 14:34, Daniel P. Berrangé wrote:
>>> On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
>>>> Storing the lseek() result in an int results in it overflowing when the
>>>> file is at least 2 GB big.  Then, we have a 50 % chance of the result
>>>> being "negative" and thus thinking an error occurred when actually
>>>> everything went just fine.
>>>>
>>>> So we should use the correct type for storing the result: off_t.
>>>>
>>>> Reported-by: Daniel P. Berrange <berrange@redhat.com>
>>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
>>>> Cc: qemu-stable@nongnu.org
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>  block/file-posix.c | 5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/block/file-posix.c b/block/file-posix.c
>>>> index f1591c3849..90c25864a0 100644
>>>> --- a/block/file-posix.c
>>>> +++ b/block/file-posix.c
>>>> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>>>>      case PREALLOC_MODE_FULL:
>>>>      {
>>>>          int64_t num = 0, left = offset - current_length;
>>>> +        off_t seek_result;
>>>>  
>>>>          /*
>>>>           * Knowing the final size from the beginning could allow the file
>>>> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>>>>  
>>>>          buf = g_malloc0(65536);
>>>>  
>>>> -        result = lseek(fd, current_length, SEEK_SET);
>>>> -        if (result < 0) {
>>>> +        seek_result = lseek(fd, current_length, SEEK_SET);
>>>> +        if (seek_result < 0) {
>>>
>>> off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
>>> exact value (off_t)-1 indicates an error. So this needs to be
>>>
>>>    if (seek_result == (off_t)-1) {
>>>       ...
>>>    }
>>
>> Hmmm... On my system, it appears to be a long int[1].  And
>> find_allocation() does an off_t < 0 comparison already.  And
>> "man 0p sys_types.h" says "blkcnt_t and off_t shall be signed integer
>> types."
> 
> Hmm, that's odd then - lseek man page explicitly said it must be cast,
> which suggested to me it could be unsigned:
> 
>    RETURN VALUE
>        Upon successful completion, lseek() returns the resulting offset  loca‐
>        tion  as  measured  in bytes from the beginning of the file.  On error,
>        the value (off_t) -1 is returned and  errno  is  set  to  indicate  the
>        error.
> 
> CC'ing Eric for the "official" POSIX answer....

But it also says (under NOTES):

The off_t data type is a signed integer data type specified by POSIX.1.

Max


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

Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 02:55:22PM +0100, Max Reitz wrote:
> On 2018-02-28 14:53, Daniel P. Berrangé wrote:
> > On Wed, Feb 28, 2018 at 02:45:49PM +0100, Max Reitz wrote:
> >> On 2018-02-28 14:34, Daniel P. Berrangé wrote:
> >>> On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
> >>>> Storing the lseek() result in an int results in it overflowing when the
> >>>> file is at least 2 GB big.  Then, we have a 50 % chance of the result
> >>>> being "negative" and thus thinking an error occurred when actually
> >>>> everything went just fine.
> >>>>
> >>>> So we should use the correct type for storing the result: off_t.
> >>>>
> >>>> Reported-by: Daniel P. Berrange <berrange@redhat.com>
> >>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
> >>>> Cc: qemu-stable@nongnu.org
> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>>> ---
> >>>>  block/file-posix.c | 5 +++--
> >>>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/block/file-posix.c b/block/file-posix.c
> >>>> index f1591c3849..90c25864a0 100644
> >>>> --- a/block/file-posix.c
> >>>> +++ b/block/file-posix.c
> >>>> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
> >>>>      case PREALLOC_MODE_FULL:
> >>>>      {
> >>>>          int64_t num = 0, left = offset - current_length;
> >>>> +        off_t seek_result;
> >>>>  
> >>>>          /*
> >>>>           * Knowing the final size from the beginning could allow the file
> >>>> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
> >>>>  
> >>>>          buf = g_malloc0(65536);
> >>>>  
> >>>> -        result = lseek(fd, current_length, SEEK_SET);
> >>>> -        if (result < 0) {
> >>>> +        seek_result = lseek(fd, current_length, SEEK_SET);
> >>>> +        if (seek_result < 0) {
> >>>
> >>> off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
> >>> exact value (off_t)-1 indicates an error. So this needs to be
> >>>
> >>>    if (seek_result == (off_t)-1) {
> >>>       ...
> >>>    }
> >>
> >> Hmmm... On my system, it appears to be a long int[1].  And
> >> find_allocation() does an off_t < 0 comparison already.  And
> >> "man 0p sys_types.h" says "blkcnt_t and off_t shall be signed integer
> >> types."
> > 
> > Hmm, that's odd then - lseek man page explicitly said it must be cast,
> > which suggested to me it could be unsigned:
> > 
> >    RETURN VALUE
> >        Upon successful completion, lseek() returns the resulting offset  loca‐
> >        tion  as  measured  in bytes from the beginning of the file.  On error,
> >        the value (off_t) -1 is returned and  errno  is  set  to  indicate  the
> >        error.
> > 
> > CC'ing Eric for the "official" POSIX answer....
> 
> But it also says (under NOTES):
> 
> The off_t data type is a signed integer data type specified by POSIX.1.

Ok, lets ignore my comments then -the "< 0" vs "!= -1" difference is
harmless given this.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate

[Eric Blake]

On 02/28/2018 07:53 AM, Daniel P. Berrangé wrote:

>>>> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>>>>   
>>>>           buf = g_malloc0(65536);
>>>>   
>>>> -        result = lseek(fd, current_length, SEEK_SET);
>>>> -        if (result < 0) {
>>>> +        seek_result = lseek(fd, current_length, SEEK_SET);
>>>> +        if (seek_result < 0) {
>>>
>>> off_t is an unsigned type, so this comparison to "< 0" is bogus - only the
>>> exact value (off_t)-1 indicates an error. So this needs to be
>>>
>>>     if (seek_result == (off_t)-1) {
>>>        ...
>>>     }

No, off_t is ALWAYS signed, even when it is 32-bit.  The cast helps code 
that is written for 64-bit off_t to still work when compiled with the 
small file model where a 32-bit value is used (but we don't have to 
worry about that, as we always compile for large file mode with 64-bit 
off_t).

>>
>> Hmmm... On my system, it appears to be a long int[1].  And
>> find_allocation() does an off_t < 0 comparison already.  And
>> "man 0p sys_types.h" says "blkcnt_t and off_t shall be signed integer
>> types."
> 
> Hmm, that's odd then - lseek man page explicitly said it must be cast,
> which suggested to me it could be unsigned:
> 
>     RETURN VALUE
>         Upon successful completion, lseek() returns the resulting offset  loca‐
>         tion  as  measured  in bytes from the beginning of the file.  On error,
>         the value (off_t) -1 is returned and  errno  is  set  to  indicate  the
>         error.
> 
> CC'ing Eric for the "official" POSIX answer....

It MAY be that the man page mentions a cast because '-1' is an int but 
'(off_t) -1' can be larger than an int.  But it may also be that you've 
uncovered something worth reporting as a bug to the man page project.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 1/2] block/file-posix: Fix fully preallocated truncate

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 02:13:14PM +0100, Max Reitz wrote:
> Storing the lseek() result in an int results in it overflowing when the
> file is at least 2 GB big.  Then, we have a 50 % chance of the result
> being "negative" and thus thinking an error occurred when actually
> everything went just fine.
> 
> So we should use the correct type for storing the result: off_t.
> 
> Reported-by: Daniel P. Berrange <berrange@redhat.com>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1549231
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/file-posix.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index f1591c3849..90c25864a0 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1697,6 +1697,7 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>      case PREALLOC_MODE_FULL:
>      {
>          int64_t num = 0, left = offset - current_length;
> +        off_t seek_result;
>  
>          /*
>           * Knowing the final size from the beginning could allow the file
> @@ -1711,8 +1712,8 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc,
>  
>          buf = g_malloc0(65536);
>  
> -        result = lseek(fd, current_length, SEEK_SET);
> -        if (result < 0) {
> +        seek_result = lseek(fd, current_length, SEEK_SET);
> +        if (seek_result < 0) {
>              result = -errno;
>              error_setg_errno(errp, -result,
>                               "Failed to seek to the old end of file");

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[Qemu-devel] [PATCH 2/2] iotests: Test preallocated truncate of 2G image

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/106     | 24 ++++++++++++++++++++++++
 tests/qemu-iotests/106.out | 10 ++++++++++
 2 files changed, 34 insertions(+)

diff --git a/tests/qemu-iotests/106 b/tests/qemu-iotests/106
index bfe71f4e60..5e51f88a78 100755
--- a/tests/qemu-iotests/106
+++ b/tests/qemu-iotests/106
@@ -86,6 +86,30 @@ for growth_mode in falloc full off; do
     $QEMU_IMG resize -f "$IMGFMT" --shrink --preallocation=$growth_mode "$TEST_IMG" -${GROWTH_SIZE}K
 done
 
+echo
+echo '=== Testing image growth on 2G empty image ==='
+
+for growth_mode in falloc full; do
+    echo
+    echo "--- growth_mode=$growth_mode ---"
+
+    # Maybe we want to do an lseek() to the end of the file before the
+    # preallocation; if the file has a length of 2 GB, that would
+    # return an integer that overflows to negative when put into a
+    # plain int.  We should use the correct type for the result, and
+    # this tests we do.
+
+    _make_test_img 2G
+    $QEMU_IMG resize -f "$IMGFMT" --preallocation=$growth_mode "$TEST_IMG" +${GROWTH_SIZE}K
+
+    actual_size=$($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep 'disk size')
+    actual_size=$(echo "$actual_size" | sed -e 's/^[^0-9]*\([0-9]\+\).*$/\1/')
+
+    if [ $actual_size -lt $GROWTH_SIZE ]; then
+        echo "ERROR: Image should have at least ${GROWTH_SIZE}K, but has ${actual_size}K"
+    fi
+done
+
 # success, all done
 echo '*** done'
 rm -f $seq.full
diff --git a/tests/qemu-iotests/106.out b/tests/qemu-iotests/106.out
index 0a42312301..c459957660 100644
--- a/tests/qemu-iotests/106.out
+++ b/tests/qemu-iotests/106.out
@@ -47,4 +47,14 @@ qemu-img: Preallocation can only be used for growing images
 
 --- growth_mode=off ---
 Image resized.
+
+=== Testing image growth on 2G empty image ===
+
+--- growth_mode=falloc ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
+Image resized.
+
+--- growth_mode=full ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
+Image resized.
 *** done
-- 
2.14.3

Re: [Qemu-devel] [PATCH 2/2] iotests: Test preallocated truncate of 2G image

[Daniel P. Berrangé]

On Wed, Feb 28, 2018 at 02:13:15PM +0100, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/106     | 24 ++++++++++++++++++++++++
>  tests/qemu-iotests/106.out | 10 ++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/tests/qemu-iotests/106 b/tests/qemu-iotests/106
> index bfe71f4e60..5e51f88a78 100755
> --- a/tests/qemu-iotests/106
> +++ b/tests/qemu-iotests/106
> @@ -86,6 +86,30 @@ for growth_mode in falloc full off; do
>      $QEMU_IMG resize -f "$IMGFMT" --shrink --preallocation=$growth_mode "$TEST_IMG" -${GROWTH_SIZE}K
>  done
>  
> +echo
> +echo '=== Testing image growth on 2G empty image ==='
> +
> +for growth_mode in falloc full; do
> +    echo
> +    echo "--- growth_mode=$growth_mode ---"
> +
> +    # Maybe we want to do an lseek() to the end of the file before the
> +    # preallocation; if the file has a length of 2 GB, that would
> +    # return an integer that overflows to negative when put into a
> +    # plain int.  We should use the correct type for the result, and
> +    # this tests we do.
> +
> +    _make_test_img 2G
> +    $QEMU_IMG resize -f "$IMGFMT" --preallocation=$growth_mode "$TEST_IMG" +${GROWTH_SIZE}K
> +
> +    actual_size=$($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep 'disk size')
> +    actual_size=$(echo "$actual_size" | sed -e 's/^[^0-9]*\([0-9]\+\).*$/\1/')
> +
> +    if [ $actual_size -lt $GROWTH_SIZE ]; then
> +        echo "ERROR: Image should have at least ${GROWTH_SIZE}K, but has ${actual_size}K"
> +    fi
> +done
> +
>  # success, all done
>  echo '*** done'
>  rm -f $seq.full
> diff --git a/tests/qemu-iotests/106.out b/tests/qemu-iotests/106.out
> index 0a42312301..c459957660 100644
> --- a/tests/qemu-iotests/106.out
> +++ b/tests/qemu-iotests/106.out
> @@ -47,4 +47,14 @@ qemu-img: Preallocation can only be used for growing images
>  
>  --- growth_mode=off ---
>  Image resized.
> +
> +=== Testing image growth on 2G empty image ===
> +
> +--- growth_mode=falloc ---
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
> +Image resized.
> +
> +--- growth_mode=full ---
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
> +Image resized.
>  *** done

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate

[Eric Blake]

On 02/28/2018 07:13 AM, Max Reitz wrote:
> Fully preallocated truncation has a 50 % chance of working on images
> files over file-posix.  It works if $SIZE % 4G < 2G, and it fails
> otherwise.  To make things even more interesting, often you would not
> even notice because qemu reported success even though it did nothing
> (because after the successful lseek(), errno was still 0, so when the
> file-posix driver tried to return a negative error code, it actually
> reported success).
> 
> This issue is fixed by patch 1 in this series.  Thanks to Daniel for
> reporting!
> 
> 
> Max Reitz (2):
>    block/file-posix: Fix fully preallocated truncate
>    iotests: Test preallocated truncate of 2G image

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

gluster.c has similar code, but it assigns glfs_lseek() to an int64_t, 
so it does not have the bug.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 0/2] block/file-posix: Fix fully preallocated truncate

[Max Reitz]

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

On 2018-02-28 14:13, Max Reitz wrote:
> Fully preallocated truncation has a 50 % chance of working on images
> files over file-posix.  It works if $SIZE % 4G < 2G, and it fails
> otherwise.  To make things even more interesting, often you would not
> even notice because qemu reported success even though it did nothing
> (because after the successful lseek(), errno was still 0, so when the
> file-posix driver tried to return a negative error code, it actually
> reported success).
> 
> This issue is fixed by patch 1 in this series.  Thanks to Daniel for
> reporting!
> 
> 
> Max Reitz (2):
>   block/file-posix: Fix fully preallocated truncate
>   iotests: Test preallocated truncate of 2G image
> 
>  block/file-posix.c         |  5 +++--
>  tests/qemu-iotests/106     | 24 ++++++++++++++++++++++++
>  tests/qemu-iotests/106.out | 10 ++++++++++
>  3 files changed, 37 insertions(+), 2 deletions(-)

Applied to my block tree...  (*cough* *cough* -- I'm the worst when it
comes to keeping track of my own patches.)

Max


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