[PATCH 1/4] kvm tool: use correct function names

[PATCH 1/4] kvm tool: use correct function names

[Prasad Joshi]

The function name sect_to_l1_offset() is changed to get_l1_index() as it
returns the l1 table index rather than offset.
Also change
    - sect_to_l2_offset to get_l2_index
    - sect_to_cluster_offset to get_cluster_offset

Signed-off-by: Prasad Joshi <prasadjoshi124@gmail.com>
---
 tools/kvm/qcow.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/kvm/qcow.c b/tools/kvm/qcow.c
index c4e3e48..219fd6b 100644
--- a/tools/kvm/qcow.c
+++ b/tools/kvm/qcow.c
@@ -15,21 +15,21 @@
 #include <linux/byteorder.h>
 #include <linux/types.h>
 
-static inline uint64_t sect_to_l1_offset(struct qcow *q, uint64_t offset)
+static inline uint64_t get_l1_index(struct qcow *q, uint64_t offset)
 {
 	struct qcow1_header *header = q->header;
 
 	return offset >> (header->l2_bits + header->cluster_bits);
 }
 
-static inline uint64_t sect_to_l2_offset(struct qcow *q, uint64_t offset)
+static inline uint64_t get_l2_index(struct qcow *q, uint64_t offset)
 {
 	struct qcow1_header *header = q->header;
 
 	return (offset >> (header->cluster_bits)) & ((1 << header->l2_bits)-1);
 }
 
-static inline uint64_t sect_to_cluster_offset(struct qcow *q, uint64_t offset)
+static inline uint64_t get_cluster_offset(struct qcow *q, uint64_t offset)
 {
 	struct qcow1_header *header = q->header;
 
@@ -53,7 +53,7 @@ static int qcow1_read_sector(struct disk_image *self, uint64_t sector, void *dst
 	if (offset >= header->size)
 		goto out_error;
 
-	l1_idx		= sect_to_l1_offset(self->priv, offset);
+	l1_idx		= get_l1_index(q, offset);
 
 	if (l1_idx >= q->table.table_size)
 		goto out_error;
@@ -71,7 +71,7 @@ static int qcow1_read_sector(struct disk_image *self, uint64_t sector, void *dst
 	if (pread_in_full(q->fd, l2_table, sizeof(uint64_t) * l2_table_size, l2_table_offset) < 0)
 		goto out_error_free_l2;
 
-	l2_idx		= sect_to_l2_offset(self->priv, offset);
+	l2_idx		= get_l2_index(q, offset);
 
 	if (l2_idx >= l2_table_size)
 		goto out_error_free_l2;
@@ -81,7 +81,7 @@ static int qcow1_read_sector(struct disk_image *self, uint64_t sector, void *dst
 	if (!clust_start)
 		goto zero_sector;
 
-	clust_offset	= sect_to_cluster_offset(self->priv, offset);
+	clust_offset	= get_cluster_offset(q, offset);
 
 	if (pread_in_full(q->fd, dst, dst_len, clust_start + clust_offset) < 0)
 		goto out_error_free_l2;
-- 
1.7.1

[PATCH 2/4] kvm tool: avoid byte-order conversion during each read operation

[Prasad Joshi]

Signed-off-by: Prasad Joshi <prasadjoshi124@gmail.com>
---
 tools/kvm/qcow.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/tools/kvm/qcow.c b/tools/kvm/qcow.c
index 219fd6b..243bfa8 100644
--- a/tools/kvm/qcow.c
+++ b/tools/kvm/qcow.c
@@ -58,7 +58,7 @@ static int qcow1_read_sector(struct disk_image *self, uint64_t sector, void *dst
 	if (l1_idx >= q->table.table_size)
 		goto out_error;
 
-	l2_table_offset	= be64_to_cpu(q->table.l1_table[l1_idx]);
+	l2_table_offset	= q->table.l1_table[l1_idx];
 	if (!l2_table_offset)
 		goto zero_sector;
 
@@ -128,16 +128,23 @@ struct disk_image_operations qcow1_disk_ops = {
 static int qcow_read_l1_table(struct qcow *q)
 {
 	struct qcow1_header *header = q->header;
+	struct qcow_table *table = &q->table;
+	u64 i;
 
-	q->table.table_size	= header->size / ((1 << header->l2_bits) * (1 << header->cluster_bits));
+	table->table_size = header->size / ((1 << header->l2_bits) *
+			(1 << header->cluster_bits));
 
-	q->table.l1_table	= calloc(q->table.table_size, sizeof(uint64_t));
-	if (!q->table.l1_table)
+	table->l1_table	= calloc(table->table_size, sizeof(u64));
+	if (!table->l1_table)
 		return -1;
 
-	if (pread_in_full(q->fd, q->table.l1_table, sizeof(uint64_t) * q->table.table_size, header->l1_table_offset) < 0)
+	if (pread_in_full(q->fd, table->l1_table, sizeof(u64) *
+				table->table_size, header->l1_table_offset) < 0)
 		return -1;
 
+	for (i = 0; i < table->table_size; i++)
+		be64_to_cpus(&table->l1_table[i]);
+
 	return 0;
 }
 
-- 
1.7.1

[PATCH 3/4] kvm tool: deallocate the cached l1_table in qcow1_disk_close() and in error path of qcow1_probe()

[Prasad Joshi]

Signed-off-by: Prasad Joshi <prasadjoshi124@gmail.com>
---
 tools/kvm/qcow.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/tools/kvm/qcow.c b/tools/kvm/qcow.c
index 243bfa8..9b9af86 100644
--- a/tools/kvm/qcow.c
+++ b/tools/kvm/qcow.c
@@ -115,6 +115,7 @@ static void qcow1_disk_close(struct disk_image *self)
 
 	q = self->priv;
 
+	free(q->table.l1_table);
 	free(q->header);
 	free(q);
 }
@@ -200,6 +201,7 @@ error:
 	if (!q)
 		return NULL;
 
+	free(q->table.l1_table);
 	free(q->header);
 	free(q);
 
-- 
1.7.1

[PATCH 4/4] kvm tool: check the cluster boundary in the qcow read code.

[Prasad Joshi]

Add a new function qcow1_read_cluster() to read a qcow cluster size data at a
time. The function qcow1_read_sector() is modified to use the function
qcow1_read_cluster().

Signed-off-by: Prasad Joshi <prasadjoshi124@gmail.com>
---
 tools/kvm/qcow.c |  123 ++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 78 insertions(+), 45 deletions(-)

diff --git a/tools/kvm/qcow.c b/tools/kvm/qcow.c
index 9b9af86..e6d5897 100644
--- a/tools/kvm/qcow.c
+++ b/tools/kvm/qcow.c
@@ -36,68 +36,101 @@ static inline uint64_t get_cluster_offset(struct qcow *q, uint64_t offset)
 	return offset & ((1 << header->cluster_bits)-1);
 }
 
-static int qcow1_read_sector(struct disk_image *self, uint64_t sector, void *dst, uint32_t dst_len)
+static uint32_t qcow1_read_cluster(struct qcow *q, uint64_t offset, void *dst, uint32_t dst_len)
 {
-	struct qcow *q = self->priv;
 	struct qcow1_header *header = q->header;
-	uint64_t l2_table_offset;
-	uint64_t l2_table_size;
-	uint64_t clust_offset;
-	uint64_t clust_start;
-	uint64_t *l2_table;
-	uint64_t l1_idx;
-	uint64_t l2_idx;
-	uint64_t offset;
-
-	offset		= sector << SECTOR_SHIFT;
-	if (offset >= header->size)
-		goto out_error;
+	struct qcow_table *table    = &q->table;
+	uint32_t length;
+
+	uint32_t l2_table_size;
+	u64 l2_table_offset;
+	u64 *l2_table;
+	u64 l2_idx;
 
-	l1_idx		= get_l1_index(q, offset);
+	uint32_t clust_size;
+	u64 clust_offset;
+	u64 clust_start;
 
-	if (l1_idx >= q->table.table_size)
-		goto out_error;
+	u64 l1_idx;
 
-	l2_table_offset	= q->table.l1_table[l1_idx];
-	if (!l2_table_offset)
-		goto zero_sector;
+	clust_size    = 1 << header->cluster_bits;
+	l2_table_size = 1 << header->l2_bits;
 
-	l2_table_size	= 1 << header->l2_bits;
+	l1_idx = get_l1_index(q, offset);
+	if (l1_idx >= table->table_size)
+		goto error;
 
-	l2_table	= calloc(l2_table_size, sizeof(uint64_t));
-	if (!l2_table)
-		goto out_error;
+	l2_idx = get_l2_index(q, offset);
+	if (l2_idx >= l2_table_size)
+		goto error;
 
-	if (pread_in_full(q->fd, l2_table, sizeof(uint64_t) * l2_table_size, l2_table_offset) < 0)
-		goto out_error_free_l2;
+	clust_offset = get_cluster_offset(q, offset);
+	if (clust_offset >= clust_size)
+		goto error;
 
-	l2_idx		= get_l2_index(q, offset);
+	length = clust_size - clust_offset;
+	if (length > dst_len)
+		length = dst_len;
 
-	if (l2_idx >= l2_table_size)
-		goto out_error_free_l2;
+	l2_table_offset = table->l1_table[l1_idx];
+	if (!l2_table_offset) {
+		/* unallocated level 2 table: returned zeroed data */
+		memset(dst, 0, length);
+		goto out;
+	}
 
-	clust_start	= be64_to_cpu(l2_table[l2_idx]);
+	l2_table = calloc(l2_table_size, sizeof(u64));
+	if (!l2_table)
+		goto error;
 
-	if (!clust_start)
-		goto zero_sector;
+	if (pread_in_full(q->fd, l2_table, sizeof(u64) * l2_table_size,
+				l2_table_offset) < 0)
+		goto free_l2;
 
-	clust_offset	= get_cluster_offset(q, offset);
+	clust_start = be64_to_cpu(l2_table[l2_idx]);
+	if (!clust_start) {
+		/* unalloacted cluster return zeroed data */
+		memset(dst, 0, length);
+		free(l2_table);
+		goto out;
+	}
 
-	if (pread_in_full(q->fd, dst, dst_len, clust_start + clust_offset) < 0)
-		goto out_error_free_l2;
+	if (pread_in_full(q->fd, dst, length, clust_start + clust_offset) < 0)
+		goto free_l2;
 
+out:
+	return length;
+free_l2:
 	free(l2_table);
-
+error:
 	return 0;
+}
 
-zero_sector:
-	memset(dst, 0, dst_len);
+static int qcow1_read_sector(struct disk_image *self, uint64_t sector, void *dst, uint32_t dst_len)
+{
+	struct qcow *q = self->priv;
+	struct qcow1_header *header = q->header;
+	uint32_t length = 0;
+	char *buf = dst;
+	uint64_t offset;
+	uint32_t nr;
 
-	return 0;
+	while (length < dst_len) {
+		offset = sector << SECTOR_SHIFT;
+		if (offset >= header->size)
+			goto error;
 
-out_error_free_l2:
-	free(l2_table);
-out_error:
+		nr = qcow1_read_cluster(q, offset, buf, dst_len - length);
+		if (!nr)
+			goto error;
+
+		length += nr;
+		buf    += nr;
+		sector += (nr >> SECTOR_SHIFT);
+	}
+
+	return 0;
+error:
 	return -1;
 }
 
-- 
1.7.1

Re: [PATCH 4/4] kvm tool: check the cluster boundary in the qcow read code.

[Pekka Enberg]

On Fri, Apr 15, 2011 at 5:18 PM, Prasad Joshi <prasadjoshi124@gmail.com> wrote:
> Add a new function qcow1_read_cluster() to read a qcow cluster size data at a
> time. The function qcow1_read_sector() is modified to use the function
> qcow1_read_cluster().
>
> Signed-off-by: Prasad Joshi <prasadjoshi124@gmail.com>

I applied patches 1-3 but this patch needs splitting:

  - s/uint64_t/u64/ fixes need to be separate

  - checking for out of bound indices

  - reads that pass cluster bondaries

                        Pekka

Re: [PATCH 4/4] kvm tool: check the cluster boundary in the qcow read code.

[Prasad Joshi]

On Sat, Apr 16, 2011 at 10:23 AM, Pekka Enberg <penberg@kernel.org> wrote:
> On Fri, Apr 15, 2011 at 5:18 PM, Prasad Joshi <prasadjoshi124@gmail.com> wrote:
>> Add a new function qcow1_read_cluster() to read a qcow cluster size data at a
>> time. The function qcow1_read_sector() is modified to use the function
>> qcow1_read_cluster().
>>
>> Signed-off-by: Prasad Joshi <prasadjoshi124@gmail.com>
>
> I applied patches 1-3 but this patch needs splitting:
>
>  - s/uint64_t/u64/ fixes need to be separate

Okay

>
>  - checking for out of bound indices

Only new check added is to verify that the cluster offset is less than
the size of the cluster.

Can this one liner changed be combined with the patch that checks for
cluster boundary?

Checking for cluster boundary adds a new function and the most of the
code from qcow1_read_sector() will be moved to qcow1_read_cluster().

Thanks and Regards,
Prasad
>
>  - reads that pass cluster bondaries
>
>                        Pekka
>