[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 3/3] dmg: don't skip zero chunk
From: |
Julio Faracco |
Subject: |
Re: [Qemu-devel] [PATCH v2 3/3] dmg: don't skip zero chunk |
Date: |
Mon, 24 Dec 2018 13:28:28 -0200 |
Looks good to me.
Reviewed-by: Julio Faracco <address@hidden>
Em dom, 23 de dez de 2018 às 01:03, yuchenlin <address@hidden>
escreveu:
> The dmg file has many tables which describe: "start from sector XXX to
> sector XXX, the compression method is XXX and where the compressed data
> resides on".
>
> Each sector in the expanded file should be covered by a table. The table
> will describe the offset of compressed data (or raw depends on the type)
> in the dmg.
>
> For example:
>
> [-----------The expanded file------------]
> [---bzip table ---]/* zeros */[---zlib---]
> ^
> | if we want to read this sector.
>
> we will find bzip table which contains this sector, and get the
> compressed data offset, read it from dmg, uncompress it, finally write to
> expanded file.
>
> If we skip zero chunk (table), some sector cannot find the table which
> will cause search_chunk() return s->n_chunks, dmg_read_chunk() return -1
> and finally causing dmg_co_preadv() return EIO.
>
> See:
>
> [-----------The expanded file------------]
> [---bzip table ---]/* zeros */[---zlib---]
> ^
> | if we want to read this sector.
>
> Oops, we cannot find the table contains it...
>
> In the original implementation, we don't have zero table. When we try to
> read sector inside the zero chunk. We will get EIO, and skip reading.
>
> After this patch, we treat zero chunk the same as ignore chunk, it will
> directly write zero and avoid some sector may not find the table.
>
> After this patch:
>
> [-----------The expanded file------------]
> [---bzip table ---][--zeros--][---zlib---]
>
> Signed-off-by: yuchenlin <address@hidden>
> ---
> block/dmg.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/block/dmg.c b/block/dmg.c
> index 6b0a057bf8..137fe9c1ff 100644
> --- a/block/dmg.c
> +++ b/block/dmg.c
> @@ -130,7 +130,8 @@ static void update_max_chunk_size(BDRVDMGState *s,
> uint32_t chunk,
> case UDRW: /* copy */
> uncompressed_sectors = DIV_ROUND_UP(s->lengths[chunk], 512);
> break;
> - case UDIG: /* zero */
> + case UDZE: /* zero */
> + case UDIG: /* ignore */
> /* as the all-zeroes block may be large, it is treated specially:
> the
> * sector is not copied from a large buffer, a simple memset is
> used
> * instead. Therefore uncompressed_sectors does not need to be
> set. */
> @@ -199,8 +200,9 @@ typedef struct DmgHeaderState {
> static bool dmg_is_known_block_type(uint32_t entry_type)
> {
> switch (entry_type) {
> + case UDZE: /* zeros */
> case UDRW: /* uncompressed */
> - case UDIG: /* zeroes */
> + case UDIG: /* ignore */
> case UDZO: /* zlib */
> return true;
> case UDBZ: /* bzip2 */
> @@ -265,9 +267,10 @@ static int dmg_read_mish_block(BDRVDMGState *s,
> DmgHeaderState *ds,
> /* sector count */
> s->sectorcounts[i] = buff_read_uint64(buffer, offset + 0x10);
>
> - /* all-zeroes sector (type 2) does not need to be "uncompressed"
> and can
> - * therefore be unbounded. */
> - if (s->types[i] != UDIG && s->sectorcounts[i] >
> DMG_SECTORCOUNTS_MAX) {
> + /* all-zeroes sector (type UDZE and UDIG) does not need to be
> + * "uncompressed" and can therefore be unbounded. */
> + if (s->types[i] != UDZE && s->types[i] != UDIG
> + && s->sectorcounts[i] > DMG_SECTORCOUNTS_MAX) {
> error_report("sector count %" PRIu64 " for chunk %" PRIu32
> " is larger than max (%u)",
> s->sectorcounts[i], i, DMG_SECTORCOUNTS_MAX);
> @@ -671,7 +674,8 @@ static inline int dmg_read_chunk(BlockDriverState *bs,
> uint64_t sector_num)
> return -1;
> }
> break;
> - case UDIG: /* zero */
> + case UDZE: /* zeros */
> + case UDIG: /* ignore */
> /* see dmg_read, it is treated specially. No buffer needs to
> be
> * pre-filled, the zeroes can be set directly. */
> break;
> @@ -706,7 +710,8 @@ dmg_co_preadv(BlockDriverState *bs, uint64_t offset,
> uint64_t bytes,
> /* Special case: current chunk is all zeroes. Do not perform a
> memcpy as
> * s->uncompressed_chunk may be too small to cover the large
> all-zeroes
> * section. dmg_read_chunk is called to find s->current_chunk */
> - if (s->types[s->current_chunk] == UDIG) { /* all zeroes block
> entry */
> + if (s->types[s->current_chunk] == UDZE
> + || s->types[s->current_chunk] == UDIG) { /* all zeroes block
> entry */
> qemu_iovec_memset(qiov, i * 512, 0, 512);
> continue;
> }
> --
> 2.17.1
>
>
>