Re: [Qemu-devel] [PATCH 3/5] qcow2: Employ metadata overlap checks

[Kevin Wolf <kwolf@redhat.com>]

Am 27.08.2013 um 13:41 hat Max Reitz geschrieben:
> Am 27.08.2013 13:32, schrieb Kevin Wolf:
> >Am 26.08.2013 um 15:04 hat Max Reitz geschrieben:
> >>The pre-write overlap check function is now called before most of the
> >>qcow2 writes (aborting it on collision or other error).
> >>
> >>Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>---
> >>  block/qcow2-cache.c    | 17 +++++++++++++++++
> >>  block/qcow2-cluster.c  | 23 +++++++++++++++++++++++
> >>  block/qcow2-snapshot.c | 24 ++++++++++++++++++++++++
> >>  block/qcow2.c          | 38 +++++++++++++++++++++++++++++++++++++-
> >>  4 files changed, 101 insertions(+), 1 deletion(-)
> >>@@ -368,6 +384,13 @@ static int coroutine_fn copy_sectors(BlockDriverState *bs,
> >>                          &s->aes_encrypt_key);
> >>      }
> >>+    ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
> >>+            ((cluster_offset >> 9) + n_start) << 9, n * BDRV_SECTOR_SIZE);
> >Looks a bit overcomplicated, I'd like something like this better:
> >
> >cluster_offset + n_start * BDRV_SECTOR_SIZE
> Yes, but this wouldn't correspond with the write call if
> (cluster_offset & ((1 << 9) - 1)) != 0. ;-)

And then you have a problem anyway. It's something that I'd be happy to
assert() at any time, i.e. if it isn't true, it's a bug.

> Basically, I just wanted it to match exactly the write command.

I can see your point. Well, matter of taste, I guess.

> >
> >>+    if (ret) {
> >>+        ret = (ret < 0) ? ret : -EIO;
> >I wonder whether the -EIO logic should be moved into
> >qcow2_pre_write_overlap_check(). Currently each single caller seems to
> >have this check.
> Seems reasonable. I didn't want to prevent the caller from receiving
> information about the exact overlap, but that could be achieved
> through an optional result pointer as well, I think.

Don't complicate an interface for a potential caller that doesn't exist
yet. If one comes up, it will change the interface as it needs.

Kevin