[PATCH 0/3] qcow2: Misc BDRV_SECTOR_SIZE updates

[PATCH 0/3] qcow2: Misc BDRV_SECTOR_SIZE updates

[Alberto Garcia]

This small series gets rid of all the remaining instances of hardcoded
sector sizes in the qcow2 code and adds a check for images whose
virtual size is not a multiple of the sector size.

See the individual patches for details.

Berto

Alberto Garcia (3):
  qcow2: Require that the virtual size is a multiple of the sector size
  qcow2: Don't round the L1 table allocation up to the sector size
  qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value

 block/qcow2-cluster.c      |  7 +++----
 block/qcow2-refcount.c     |  2 +-
 block/qcow2-snapshot.c     |  3 +--
 block/qcow2.c              | 25 +++++++++++++++++--------
 docs/interop/qcow2.txt     |  3 ++-
 tests/qemu-iotests/080     |  7 +++++++
 tests/qemu-iotests/080.out |  4 ++++
 7 files changed, 35 insertions(+), 16 deletions(-)

-- 
2.20.1

[PATCH 1/3] qcow2: Require that the virtual size is a multiple of the sector size

[Alberto Garcia]

The qcow2 header specifies the virtual size of the image in bytes, but
BlockDriverState stores it as a number of 512-byte sectors.

If the user tries to create an image with a size that is not a
multiple of the sector size then this is fixed on creation by
silently rounding the image size up (see commit c2eb918e32).
qcow2_co_truncate() is more strict and returns an error instead.

However when an image is opened the virtual size is rounded down,
which means that trying to access the last few advertised bytes will
result in an error. As seen above QEMU cannot create such images and
there's no good use case that would require us to try to handle them
so let's just treat them as unsupported.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c              | 7 +++++++
 docs/interop/qcow2.txt     | 3 ++-
 tests/qemu-iotests/080     | 7 +++++++
 tests/qemu-iotests/080.out | 4 ++++
 4 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 7fbaac8457..92474849db 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1326,6 +1326,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
         goto fail;
     }
 
+    if (header.size % BDRV_SECTOR_SIZE) {
+        error_setg(errp, "Virtual size is not a multiple of %u",
+                   (unsigned) BDRV_SECTOR_SIZE);
+        ret = -EINVAL;
+        goto fail;
+    }
+
     if (header.header_length > sizeof(header)) {
         s->unknown_header_fields_size = header.header_length - sizeof(header);
         s->unknown_header_fields = g_malloc(s->unknown_header_fields_size);
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index af5711e533..891f5662d8 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -40,7 +40,8 @@ The first cluster of a qcow2 image contains the file header:
                     with larger cluster sizes.
 
          24 - 31:   size
-                    Virtual disk size in bytes.
+                    Virtual disk size in bytes. qemu can only handle
+                    sizes that are a multiple of 512 bytes.
 
                     Note: qemu has an implementation limit of 32 MB as
                     the maximum L1 table size.  With a 2 MB cluster
diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
index 4bcb5021e8..2563b2c052 100755
--- a/tests/qemu-iotests/080
+++ b/tests/qemu-iotests/080
@@ -48,6 +48,7 @@ header_size=104
 
 offset_backing_file_offset=8
 offset_backing_file_size=16
+offset_virtual_size=24
 offset_l1_size=36
 offset_l1_table_offset=40
 offset_refcount_table_offset=48
@@ -197,6 +198,12 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_size" "\x10\x00\x00\x00"
 { $QEMU_IMG snapshot -d test $TEST_IMG; } 2>&1 | _filter_testdir
 _check_test_img
 
+echo
+echo "== Image size not a multiple of the sector size =="
+_make_test_img 64k
+poke_file "$TEST_IMG" "$offset_virtual_size" "\x00\x00\x00\x00\x00\x00\xff\xff"
+{ $QEMU_IO -c "write 65530 1" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
index 45ab01db8e..e1c969e2ba 100644
--- a/tests/qemu-iotests/080.out
+++ b/tests/qemu-iotests/080.out
@@ -104,4 +104,8 @@ Data may be corrupted, or further writes to the image may corrupt it.
 
 3 leaked clusters were found on the image.
 This means waste of disk space, but no harm to data.
+
+== Image size not a multiple of the sector size ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
+qemu-io: can't open device TEST_DIR/t.qcow2: Virtual size is not a multiple of 512
 *** done
-- 
2.20.1

[PATCH 2/3] qcow2: Don't round the L1 table allocation up to the sector size

[Alberto Garcia]

The L1 table is read from disk using the byte-based bdrv_pread() and
is never accessed beyond its last element, so there's no need to
allocate more memory than that.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2-cluster.c  | 5 ++---
 block/qcow2-refcount.c | 2 +-
 block/qcow2-snapshot.c | 3 +--
 block/qcow2.c          | 2 +-
 4 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 8982b7b762..932fc48919 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -124,12 +124,11 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
 #endif
 
     new_l1_size2 = sizeof(uint64_t) * new_l1_size;
-    new_l1_table = qemu_try_blockalign(bs->file->bs,
-                                       ROUND_UP(new_l1_size2, 512));
+    new_l1_table = qemu_try_blockalign(bs->file->bs, new_l1_size2);
     if (new_l1_table == NULL) {
         return -ENOMEM;
     }
-    memset(new_l1_table, 0, ROUND_UP(new_l1_size2, 512));
+    memset(new_l1_table, 0, new_l1_size2);
 
     if (s->l1_size) {
         memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index f67ac6b2d8..c963bc8de1 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1262,7 +1262,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
      * l1_table_offset when it is the current s->l1_table_offset! Be careful
      * when changing this! */
     if (l1_table_offset != s->l1_table_offset) {
-        l1_table = g_try_malloc0(ROUND_UP(l1_size2, 512));
+        l1_table = g_try_malloc0(l1_size2);
         if (l1_size2 && l1_table == NULL) {
             ret = -ENOMEM;
             goto fail;
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5ab64da1ec..82c32d4c9b 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -1024,8 +1024,7 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
         return ret;
     }
     new_l1_bytes = sn->l1_size * sizeof(uint64_t);
-    new_l1_table = qemu_try_blockalign(bs->file->bs,
-                                       ROUND_UP(new_l1_bytes, 512));
+    new_l1_table = qemu_try_blockalign(bs->file->bs, new_l1_bytes);
     if (new_l1_table == NULL) {
         return -ENOMEM;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 92474849db..e8ce966f7f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1499,7 +1499,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
 
     if (s->l1_size > 0) {
         s->l1_table = qemu_try_blockalign(bs->file->bs,
-            ROUND_UP(s->l1_size * sizeof(uint64_t), 512));
+                                          s->l1_size * sizeof(uint64_t));
         if (s->l1_table == NULL) {
             error_setg(errp, "Could not allocate L1 table");
             ret = -ENOMEM;
-- 
2.20.1

[PATCH 3/3] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value

[Alberto Garcia]

This replaces all remaining instances in the qcow2 code.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2-cluster.c |  2 +-
 block/qcow2.c         | 16 +++++++++-------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 932fc48919..777ca2d409 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -219,7 +219,7 @@ static int l2_load(BlockDriverState *bs, uint64_t offset,
  * Writes one sector of the L1 table to the disk (can't update single entries
  * and we really don't want bdrv_pread to perform a read-modify-write)
  */
-#define L1_ENTRIES_PER_SECTOR (512 / 8)
+#define L1_ENTRIES_PER_SECTOR (BDRV_SECTOR_SIZE / 8)
 int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)
 {
     BDRVQcow2State *s = bs->opaque;
diff --git a/block/qcow2.c b/block/qcow2.c
index e8ce966f7f..6427c75409 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2175,7 +2175,7 @@ static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
                                           offset, bytes, qiov, qiov_offset);
 
     case QCOW2_CLUSTER_NORMAL:
-        if ((file_cluster_offset & 511) != 0) {
+        if ((file_cluster_offset % BDRV_SECTOR_SIZE) != 0) {
             return -EIO;
         }
 
@@ -2514,7 +2514,7 @@ static coroutine_fn int qcow2_co_pwritev_part(
             goto out_locked;
         }
 
-        assert((cluster_offset & 511) == 0);
+        assert((cluster_offset % BDRV_SECTOR_SIZE) == 0);
 
         ret = qcow2_pre_write_overlap_check(bs, 0,
                                             cluster_offset + offset_in_cluster,
@@ -3283,7 +3283,8 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
 
     /* Validate options and set default values */
     if (!QEMU_IS_ALIGNED(qcow2_opts->size, BDRV_SECTOR_SIZE)) {
-        error_setg(errp, "Image size must be a multiple of 512 bytes");
+        error_setg(errp, "Image size must be a multiple of %u bytes",
+                   (unsigned) BDRV_SECTOR_SIZE);
         ret = -EINVAL;
         goto out;
     }
@@ -3839,7 +3840,7 @@ qcow2_co_copy_range_from(BlockDriverState *bs,
         case QCOW2_CLUSTER_NORMAL:
             child = s->data_file;
             copy_offset += offset_into_cluster(s, src_offset);
-            if ((copy_offset & 511) != 0) {
+            if ((copy_offset % BDRV_SECTOR_SIZE) != 0) {
                 ret = -EIO;
                 goto out;
             }
@@ -3904,7 +3905,7 @@ qcow2_co_copy_range_to(BlockDriverState *bs,
             goto fail;
         }
 
-        assert((cluster_offset & 511) == 0);
+        assert((cluster_offset % BDRV_SECTOR_SIZE) == 0);
 
         ret = qcow2_pre_write_overlap_check(bs, 0,
                 cluster_offset + offset_in_cluster, cur_bytes, true);
@@ -3961,8 +3962,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
         return -ENOTSUP;
     }
 
-    if (offset & 511) {
-        error_setg(errp, "The new size must be a multiple of 512");
+    if (offset % BDRV_SECTOR_SIZE) {
+        error_setg(errp, "The new size must be a multiple of %u",
+                   (unsigned) BDRV_SECTOR_SIZE);
         return -EINVAL;
     }
 
-- 
2.20.1

Re: [PATCH 3/3] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value

[Nir Soffer]

On Wed, Jan 8, 2020 at 7:50 PM Alberto Garcia <berto@igalia.com> wrote:
>
> This replaces all remaining instances in the qcow2 code.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2-cluster.c |  2 +-
>  block/qcow2.c         | 16 +++++++++-------
>  2 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 932fc48919..777ca2d409 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -219,7 +219,7 @@ static int l2_load(BlockDriverState *bs, uint64_t offset,
>   * Writes one sector of the L1 table to the disk (can't update single entries
>   * and we really don't want bdrv_pread to perform a read-modify-write)
>   */
> -#define L1_ENTRIES_PER_SECTOR (512 / 8)
> +#define L1_ENTRIES_PER_SECTOR (BDRV_SECTOR_SIZE / 8)
>  int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)
>  {
>      BDRVQcow2State *s = bs->opaque;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index e8ce966f7f..6427c75409 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2175,7 +2175,7 @@ static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
>                                            offset, bytes, qiov, qiov_offset);
>
>      case QCOW2_CLUSTER_NORMAL:
> -        if ((file_cluster_offset & 511) != 0) {
> +        if ((file_cluster_offset % BDRV_SECTOR_SIZE) != 0) {

All the alignment checks should use QEMU_IS_ALIGNED.

>              return -EIO;
>          }
>
> @@ -2514,7 +2514,7 @@ static coroutine_fn int qcow2_co_pwritev_part(
>              goto out_locked;
>          }
>
> -        assert((cluster_offset & 511) == 0);
> +        assert((cluster_offset % BDRV_SECTOR_SIZE) == 0);
>
>          ret = qcow2_pre_write_overlap_check(bs, 0,
>                                              cluster_offset + offset_in_cluster,
> @@ -3283,7 +3283,8 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
>
>      /* Validate options and set default values */
>      if (!QEMU_IS_ALIGNED(qcow2_opts->size, BDRV_SECTOR_SIZE)) {
> -        error_setg(errp, "Image size must be a multiple of 512 bytes");
> +        error_setg(errp, "Image size must be a multiple of %u bytes",
> +                   (unsigned) BDRV_SECTOR_SIZE);
>          ret = -EINVAL;
>          goto out;
>      }
> @@ -3839,7 +3840,7 @@ qcow2_co_copy_range_from(BlockDriverState *bs,
>          case QCOW2_CLUSTER_NORMAL:
>              child = s->data_file;
>              copy_offset += offset_into_cluster(s, src_offset);
> -            if ((copy_offset & 511) != 0) {
> +            if ((copy_offset % BDRV_SECTOR_SIZE) != 0) {
>                  ret = -EIO;
>                  goto out;
>              }
> @@ -3904,7 +3905,7 @@ qcow2_co_copy_range_to(BlockDriverState *bs,
>              goto fail;
>          }
>
> -        assert((cluster_offset & 511) == 0);
> +        assert((cluster_offset % BDRV_SECTOR_SIZE) == 0);
>
>          ret = qcow2_pre_write_overlap_check(bs, 0,
>                  cluster_offset + offset_in_cluster, cur_bytes, true);
> @@ -3961,8 +3962,9 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
>          return -ENOTSUP;
>      }
>
> -    if (offset & 511) {
> -        error_setg(errp, "The new size must be a multiple of 512");
> +    if (offset % BDRV_SECTOR_SIZE) {
> +        error_setg(errp, "The new size must be a multiple of %u",
> +                   (unsigned) BDRV_SECTOR_SIZE);
>          return -EINVAL;
>      }
>
> --
> 2.20.1

Otherwise looks good.

Nir

Re: [PATCH 1/3] qcow2: Require that the virtual size is a multiple of the sector size

[Nir Soffer]

On Wed, Jan 8, 2020 at 7:52 PM Alberto Garcia <berto@igalia.com> wrote:
>
> The qcow2 header specifies the virtual size of the image in bytes, but
> BlockDriverState stores it as a number of 512-byte sectors.
>
> If the user tries to create an image with a size that is not a
> multiple of the sector size then this is fixed on creation by
> silently rounding the image size up (see commit c2eb918e32).
> qcow2_co_truncate() is more strict and returns an error instead.
>
> However when an image is opened the virtual size is rounded down,
> which means that trying to access the last few advertised bytes will
> result in an error. As seen above QEMU cannot create such images and
> there's no good use case that would require us to try to handle them
> so let's just treat them as unsupported.

Making error impossible is best.

Can we require multiple of 4k to avoid unaligned read/write at the end
of an image
aligned to 512 bytes on storage with 4k sector size?

>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2.c              | 7 +++++++
>  docs/interop/qcow2.txt     | 3 ++-
>  tests/qemu-iotests/080     | 7 +++++++
>  tests/qemu-iotests/080.out | 4 ++++
>  4 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7fbaac8457..92474849db 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1326,6 +1326,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>          goto fail;
>      }
>
> +    if (header.size % BDRV_SECTOR_SIZE) {
> +        error_setg(errp, "Virtual size is not a multiple of %u",
> +                   (unsigned) BDRV_SECTOR_SIZE);
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
>      if (header.header_length > sizeof(header)) {
>          s->unknown_header_fields_size = header.header_length - sizeof(header);
>          s->unknown_header_fields = g_malloc(s->unknown_header_fields_size);
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index af5711e533..891f5662d8 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -40,7 +40,8 @@ The first cluster of a qcow2 image contains the file header:
>                      with larger cluster sizes.
>
>           24 - 31:   size
> -                    Virtual disk size in bytes.
> +                    Virtual disk size in bytes. qemu can only handle
> +                    sizes that are a multiple of 512 bytes.
>
>                      Note: qemu has an implementation limit of 32 MB as
>                      the maximum L1 table size.  With a 2 MB cluster
> diff --git a/tests/qemu-iotests/080 b/tests/qemu-iotests/080
> index 4bcb5021e8..2563b2c052 100755
> --- a/tests/qemu-iotests/080
> +++ b/tests/qemu-iotests/080
> @@ -48,6 +48,7 @@ header_size=104
>
>  offset_backing_file_offset=8
>  offset_backing_file_size=16
> +offset_virtual_size=24
>  offset_l1_size=36
>  offset_l1_table_offset=40
>  offset_refcount_table_offset=48
> @@ -197,6 +198,12 @@ poke_file "$TEST_IMG" "$offset_snap1_l1_size" "\x10\x00\x00\x00"
>  { $QEMU_IMG snapshot -d test $TEST_IMG; } 2>&1 | _filter_testdir
>  _check_test_img
>
> +echo
> +echo "== Image size not a multiple of the sector size =="
> +_make_test_img 64k

Logging the change here would make the test and the output more clear:

    echo "modifying virtual size to 65535"

> +poke_file "$TEST_IMG" "$offset_virtual_size" "\x00\x00\x00\x00\x00\x00\xff\xff"
> +{ $QEMU_IO -c "write 65530 1" $TEST_IMG; } 2>&1 | _filter_qemu_io | _filter_testdir
> +
>  # success, all done
>  echo "*** done"
>  rm -f $seq.full
> diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
> index 45ab01db8e..e1c969e2ba 100644
> --- a/tests/qemu-iotests/080.out
> +++ b/tests/qemu-iotests/080.out
> @@ -104,4 +104,8 @@ Data may be corrupted, or further writes to the image may corrupt it.
>
>  3 leaked clusters were found on the image.
>  This means waste of disk space, but no harm to data.
> +
> +== Image size not a multiple of the sector size ==
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
> +qemu-io: can't open device TEST_DIR/t.qcow2: Virtual size is not a multiple of 512

The output is confusing, looks like we created image with aligned
size, and on the next
line it complains about the size.

>  *** done
> --
> 2.20.1
>
>

Nir

Re: [PATCH 1/3] qcow2: Require that the virtual size is a multiple of the sector size

[Kevin Wolf]

Am 08.01.2020 um 20:46 hat Nir Soffer geschrieben:
> On Wed, Jan 8, 2020 at 7:52 PM Alberto Garcia <berto@igalia.com> wrote:
> >
> > The qcow2 header specifies the virtual size of the image in bytes, but
> > BlockDriverState stores it as a number of 512-byte sectors.
> >
> > If the user tries to create an image with a size that is not a
> > multiple of the sector size then this is fixed on creation by
> > silently rounding the image size up (see commit c2eb918e32).
> > qcow2_co_truncate() is more strict and returns an error instead.
> >
> > However when an image is opened the virtual size is rounded down,
> > which means that trying to access the last few advertised bytes will
> > result in an error. As seen above QEMU cannot create such images and
> > there's no good use case that would require us to try to handle them
> > so let's just treat them as unsupported.

I still hope that we'll convert bs->total_sectors to something based on
bytes so we can actually handle byte-granularity image sizes, but for
the time being, I guess this fix makes sense (though we don't have the
check in other drivers which have the same problem).

> Making error impossible is best.
> 
> Can we require multiple of 4k to avoid unaligned read/write at the end
> of an image aligned to 512 bytes on storage with 4k sector size?

A qcow2 image should be able to provide an image for any disk the user
wants to expose to the guest, and 4k alignment would certainly limit
that ability. I suspect that if CHS geometry matters for a guest, not
having a 4k aligned size is actually quite likely.

Kevin

Re: [PATCH 3/3] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value

[Kevin Wolf]

Am 08.01.2020 um 18:49 hat Alberto Garcia geschrieben:
> This replaces all remaining instances in the qcow2 code.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

> diff --git a/block/qcow2.c b/block/qcow2.c
> index e8ce966f7f..6427c75409 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2175,7 +2175,7 @@ static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
>                                            offset, bytes, qiov, qiov_offset);
>  
>      case QCOW2_CLUSTER_NORMAL:
> -        if ((file_cluster_offset & 511) != 0) {
> +        if ((file_cluster_offset % BDRV_SECTOR_SIZE) != 0) {
>              return -EIO;
>          }

Hm, unrelated to your change, but why do we test for 512 byte alignment
here? file_cluster_offset should certainly be cluster aligned for normal
clusters. And if the check fails, that's actually an image corruption
and not just an I/O error. Am I missing something?

Kevin

Re: [PATCH 3/3] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value

[Alberto Garcia]

On Thu 09 Jan 2020 01:19:00 PM CET, Kevin Wolf wrote:
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index e8ce966f7f..6427c75409 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2175,7 +2175,7 @@ static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
>>                                            offset, bytes, qiov, qiov_offset);
>>  
>>      case QCOW2_CLUSTER_NORMAL:
>> -        if ((file_cluster_offset & 511) != 0) {
>> +        if ((file_cluster_offset % BDRV_SECTOR_SIZE) != 0) {
>>              return -EIO;
>>          }
>
> Hm, unrelated to your change, but why do we test for 512 byte
> alignment here? file_cluster_offset should certainly be cluster
> aligned for normal clusters. And if the check fails, that's actually
> an image corruption and not just an I/O error. Am I missing something?

I actually suspect that this is just an old, obsolete check that we have
kept during these years. file_cluster_offset should be not just sector
aligned but also cluster aligned if I'm not wrong, and if not then
qcow2_alloc_cluster_offset() and qcow2_get_cluster_offset() should
return an error.

I can simply remove that check, or replace it with an assertion.

Berto

Re: [PATCH 1/3] qcow2: Require that the virtual size is a multiple of the sector size

[Alberto Garcia]

On Wed 08 Jan 2020 08:46:11 PM CET, Nir Soffer wrote:
>> However when an image is opened the virtual size is rounded down,
>> which means that trying to access the last few advertised bytes will
>> result in an error. As seen above QEMU cannot create such images and
>> there's no good use case that would require us to try to handle them
>> so let's just treat them as unsupported.
>
> Making error impossible is best.
>
> Can we require multiple of 4k to avoid unaligned read/write at the end
> of an image aligned to 512 bytes on storage with 4k sector size?

I wouldn't force that on the user. The only reason why I'm not allowing
the non-sector-aligned case is because it's currently broken, but I
wouldn't have a problem with it if it was working fine.

>> +echo
>> +echo "== Image size not a multiple of the sector size =="
>> +_make_test_img 64k
>
> Logging the change here would make the test and the output more clear:
>
>     echo "modifying virtual size to 65535"

Ok

>> +== Image size not a multiple of the sector size ==
>> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
>> +qemu-io: can't open device TEST_DIR/t.qcow2: Virtual size is not a multiple of 512
>
> The output is confusing, looks like we created image with aligned
> size, and on the next line it complains about the size.

Ok, I can also replace the "qemu-io write" with a "qemu-img info" or
something like that to make it a bit less confusing (i.e. the error
happens already when you open the image, not when you write to it).

Berto

Re: [PATCH 3/3] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value

[Kevin Wolf]

Am 09.01.2020 um 13:30 hat Alberto Garcia geschrieben:
> On Thu 09 Jan 2020 01:19:00 PM CET, Kevin Wolf wrote:
> >> diff --git a/block/qcow2.c b/block/qcow2.c
> >> index e8ce966f7f..6427c75409 100644
> >> --- a/block/qcow2.c
> >> +++ b/block/qcow2.c
> >> @@ -2175,7 +2175,7 @@ static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
> >>                                            offset, bytes, qiov, qiov_offset);
> >>  
> >>      case QCOW2_CLUSTER_NORMAL:
> >> -        if ((file_cluster_offset & 511) != 0) {
> >> +        if ((file_cluster_offset % BDRV_SECTOR_SIZE) != 0) {
> >>              return -EIO;
> >>          }
> >
> > Hm, unrelated to your change, but why do we test for 512 byte
> > alignment here? file_cluster_offset should certainly be cluster
> > aligned for normal clusters. And if the check fails, that's actually
> > an image corruption and not just an I/O error. Am I missing something?
> 
> I actually suspect that this is just an old, obsolete check that we have
> kept during these years. file_cluster_offset should be not just sector
> aligned but also cluster aligned if I'm not wrong, and if not then
> qcow2_alloc_cluster_offset() and qcow2_get_cluster_offset() should
> return an error.

Right, they already check it, and don't only return an error, but also
call qcow2_signal_corruption() as they should.

> I can simply remove that check, or replace it with an assertion.

Sounds good to me (and with cluster size instead of 512).

Kevin