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 <mre...@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
