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

[Kevin Wolf <kwolf@redhat.com>]

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