On Tue, Jul 29, 2014 at 09:00:43AM +0800, Fam Zheng wrote:
> On Mon, 07/28 16:11, Stefan Hajnoczi wrote:
> > On Tue, May 27, 2014 at 04:49:22PM +0800, Fam Zheng wrote:
> > > + if (!bs->backing_hd) {
> > > + memset(whole_grain, 0, skip_start_sector << BDRV_SECTOR_BITS);
> > > + memset(whole_grain + (skip_end_sector << BDRV_SECTOR_BITS), 0,
> > > + cluster_bytes - (skip_end_sector << BDRV_SECTOR_BITS));
> > > + }
> > > +
> > > + assert(skip_end_sector <= sector_num + extent->cluster_sectors);
> >
> > Does this assertion make sense? skip_end_sector is a small number of
> > sectors (relative to start of cluster), while sector_num +
> > extent->cluster_sectors is a large absolute sector offset.
>
> skip_end_sector is absolute sector number too. The caller hunk in this patch
> is:
I disagree. If it was an absolute sector number then the memset() a few
lines above would be incorrect:
memset(whole_grain, 0, skip_start_sector << BDRV_SECTOR_BITS);
memset(whole_grain + (skip_end_sector << BDRV_SECTOR_BITS), 0,
cluster_bytes - (skip_end_sector << BDRV_SECTOR_BITS));
Look at the code you pasted again:
> @@ -1406,12 +1468,17 @@ static int vmdk_write(BlockDriverState *bs, int64_t
> sector_num,
> if (!extent) {
> return -EIO;
> }
> - ret = get_cluster_offset(
> - bs,
> - extent,
> - &m_data,
> - sector_num << 9, !extent->compressed,
> - &cluster_offset);
> + extent_begin_sector = extent->end_sector - extent->sectors;
> + extent_relative_sector_num = sector_num - extent_begin_sector;
> + index_in_cluster = extent_relative_sector_num %
> extent->cluster_sectors;
> + n = extent->cluster_sectors - index_in_cluster;
> + if (n > nb_sectors) {
> + n = nb_sectors;
> + }
> + ret = get_cluster_offset(bs, extent, &m_data, sector_num << 9,
> + !(extent->compressed || zeroed),
> + &cluster_offset,
> + index_in_cluster, index_in_cluster + n);
> if (extent->compressed) {
> if (ret == VMDK_OK) {
> /* Refuse write to allocated cluster for streamOptimized */
>
> See the last parameter of get_cluster_offset.
The last parameter is (extent_relative_sector_num %
extent->cluster_sectors) + (extent->cluster_sectors - index_in_cluster).
Those are definitely sector counts (like nb_sectors) and not absolute
sector numbers (like sector_num).
pgppqs4d2XJb8.pgp
Description: PGP signature
