From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53122) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XH9T4-0003v2-9O for qemu-devel@nongnu.org; Tue, 12 Aug 2014 06:38:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XH9Sy-0004dF-4J for qemu-devel@nongnu.org; Tue, 12 Aug 2014 06:38:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53221) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XH9Sx-0004cx-FA for qemu-devel@nongnu.org; Tue, 12 Aug 2014 06:38:08 -0400 Date: Tue, 12 Aug 2014 11:37:49 +0100 From: Stefan Hajnoczi Message-ID: <20140812103749.GH20490@stefanha-thinkpad.redhat.com> References: <1404207919-15667-1-git-send-email-ssdxiao@163.com> <1404207919-15667-2-git-send-email-ssdxiao@163.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="y9PDtDHaFrXNoMPU" Content-Disposition: inline In-Reply-To: <1404207919-15667-2-git-send-email-ssdxiao@163.com> Subject: Re: [Qemu-devel] [RFC PATCH v1] Support vhd type VHD_DIFFERENCING List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ding xiao Cc: Kevin Wolf , Xiao Ding , Luo Hao , qemu-devel --y9PDtDHaFrXNoMPU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Jul 01, 2014 at 05:45:19PM +0800, Ding xiao wrote: Sorry for the delay, I forgot about this patch. > +typedef struct vhd_tdbatmap_header { > + char magic[8]; /* "tdbatmap"*/ > + > + /* byte offset to batmap*/ > + uint64_t batmap_offset; > + > + /* Offset of the Block Allocation Table (BAT)*/ This comment describes the batmap_offset field? Maybe this should be dropped since that field already has a comment. > + /* read backend file*/ > + if (dyndisk_header->parent_name[0] || dyndisk_header->parent_nam= e[1]) { > + for (i =3D 0; i < PARENT_LOCATOR_NUM; i++) { > + data_offset =3D be64_to_cpu( > + dyndisk_header->parent_locator[i].data_o= ffset); > + data_length =3D be32_to_cpu( > + dyndisk_header->parent_locator[i].data_l= ength); > + if (dyndisk_header->parent_locator[i].platform =3D=3D MA= CX) { Missing be32_to_cpu()? > + ret =3D bdrv_pread(bs->file, data_offset + 7, > + bs->backing_file, data_length - 7); Buffer overflow: char bs->backing_file[1024]. All input must be validated! > + if (ret < 0) { > + goto fail; > + } > + bs->backing_file[data_length - 7] =3D '\0'; Memory corruption if data_length < 7. Missing input validation. > + } > + if (data_offset > parent_locator_offset) { > + parent_locator_offset =3D data_offset; > + } > + } > + } > + > + if (parent_locator_offset + 512 > s->free_data_block_offset) { > + s->free_data_block_offset =3D parent_locator_offset + 512; > + } > + > for (i =3D 0; i < s->max_table_entries; i++) { > be32_to_cpus(&s->pagetable[i]); > if (s->pagetable[i] !=3D 0xFFFFFFFF) { > @@ -364,6 +425,9 @@ static inline int64_t get_sector_offset(BlockDriverSt= ate *bs, > // bitmap each time we write to a new block. This might cause Virtua= l PC to > // miss sparse read optimization, but it's not a problem in terms of > // correctness. > + > + /*this will not use*/ > +#if 0 Delete the code if it is no longer used. > @@ -433,7 +498,7 @@ static int rewrite_footer(BlockDriverState* bs) > * > * Returns the sectors' offset in the image file on success and < 0 on e= rror > */ > -static int64_t alloc_block(BlockDriverState* bs, int64_t sector_num) > +static int64_t alloc_block(BlockDriverState *bs, int64_t sector_num, int= diff) Please use the C99 bool type since it communicates more clearly that diff is either true or false (not a counter or bitmap). It would be clearest to check footer->type for VHD_DIFFERENCING in this function instead of adding a new argument to the function. > @@ -501,33 +611,64 @@ static int vpc_read(BlockDriverState *bs, int64_t s= ector_num, > int64_t offset; > int64_t sectors, sectors_per_block; > VHDFooter *footer =3D (VHDFooter *) s->footer_buf; > + QEMUIOVector hd_qiov; > + struct iovec qiov; > =20 > if (cpu_to_be32(footer->type) =3D=3D VHD_FIXED) { > return bdrv_read(bs->file, sector_num, buf, nb_sectors); > - } > - while (nb_sectors > 0) { > - offset =3D get_sector_offset(bs, sector_num, 0); > + } else if (cpu_to_be32(footer->type) =3D=3D VHD_DYNAMIC) { cpu_to_be32() is wrong since VHD_DYNAMIC is an enum constant (just a regular CPU-endian integer). Please add a separate patch before this one that cleans up incorrect cpu_to_be*() usage. For example, cpu_to_be32(footer->type) =3D=3D VHD_FIXED a few lines above is wrong too. > + while (nb_sectors > 0) { > + offset =3D get_sector_offset(bs, sector_num, 0); > + > + sectors_per_block =3D s->block_size >> BDRV_SECTOR_BITS; > + sectors =3D sectors_per_block - (sector_num % sectors_per_bl= ock); > + if (sectors > nb_sectors) { > + sectors =3D nb_sectors; > + } > =20 > - sectors_per_block =3D s->block_size >> BDRV_SECTOR_BITS; > - sectors =3D sectors_per_block - (sector_num % sectors_per_block); > - if (sectors > nb_sectors) { > - sectors =3D nb_sectors; > + if (offset =3D=3D -1) { > + memset(buf, 0, sectors * BDRV_SECTOR_SIZE); > + } else { > + ret =3D bdrv_pread(bs->file, offset, buf, > + sectors * BDRV_SECTOR_SIZE); > + if (ret !=3D sectors * BDRV_SECTOR_SIZE) { Indentation is off > + return -1; > + } > + } > + > + nb_sectors -=3D sectors; > + sector_num +=3D sectors; > + buf +=3D sectors * BDRV_SECTOR_SIZE; > } > + } else { > + while (nb_sectors > 0) { > + offset =3D get_sector_offset_diff(bs, sector_num); > + if (offset =3D=3D -1) { > + memset(buf, 0, BDRV_SECTOR_SIZE); > + } else if (offset =3D=3D -2) { > + qiov.iov_base =3D (void *)buf; This cast is unnecessary. The compiler does not warn about pointer casts to or from void*. > + qiov.iov_len =3D 512; > + hd_qiov.iov =3D &qiov; > + hd_qiov.niov =3D 1; > + hd_qiov.nalloc =3D -1; > + hd_qiov.size =3D 512; This is not idiomatic. Normally 'qiov' is a QEMUIOVector, not a struct iovec. The qemu_iovec_*() functions should be used instead of manually setting QEMUIOVector fields: iov.iov_base =3D buf; iov.iov_len =3D 512; qemu_iovec_init_external(&hd_qiov, &iov, 1); > + ret =3D bdrv_co_readv(bs->backing_hd, sector_num, 1, &hd= _qiov); > + if (ret < 0) { > + return -1; > + } Why are you using bdrv_co_readv() instead of bdrv_pread() like the rest of this file? It would be simpler to use bdrv_pread(). Then you don't need the struct iovec and QEMUIOVector. This function also hasn't been marked coroutine_fn yet, so it is cleaner to stick with bdrv_pread() until the file is properly converted to coroutines. --y9PDtDHaFrXNoMPU Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJT6e59AAoJEJykq7OBq3PI0s4H/Ax8aUEMXlOzuhW1GU3XL5AB oCbIJywyIfS6cK/4OG9ZcCYgwTX6RftHAhGJVch+Ey2USJDQtuZ8EdTXqA/PBCbw j5kkAvK44qeI3lKmBziKcthTSpRYG/+e/j86A9QWmlHiI2ywueNZCg1gs+/0RFqP zIhRIOX1YUAyOIEdibCRWdw2btnVPPi1DDTEmWuSzM9TNrm/576rtq1yrcTa6jp4 cybIXSn+U4U0isONqOrT7/dRVUPOZqNBN82rhJZZaqgmbqM2ZCs4Ebjz3lZ78mAS xffiRWQ93fEOr9kmwe9zPmPNaKq1QyStXbnVn/JLodYqWGBOrivsFT6FCo9Sub0= =/GxE -----END PGP SIGNATURE----- --y9PDtDHaFrXNoMPU--