[PATCH 1/2 V2] kvm tools: Fix leak in QCOW

[PATCH 1/2 V2] kvm tools: Fix leak in QCOW

[Sasha Levin]

Fixed leak when reading a zero sector, also simplified flow a bit.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/qcow.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/kvm/qcow.c b/tools/kvm/qcow.c
index 9b9af86..ceac721 100644
--- a/tools/kvm/qcow.c
+++ b/tools/kvm/qcow.c
@@ -44,7 +44,7 @@ static int qcow1_read_sector(struct disk_image *self, uint64_t sector, void *dst
 	uint64_t l2_table_size;
 	uint64_t clust_offset;
 	uint64_t clust_start;
-	uint64_t *l2_table;
+	uint64_t *l2_table = NULL;
 	uint64_t l1_idx;
 	uint64_t l2_idx;
 	uint64_t offset;
@@ -69,24 +69,24 @@ static int qcow1_read_sector(struct disk_image *self, uint64_t sector, void *dst
 		goto out_error;
 
 	if (pread_in_full(q->fd, l2_table, sizeof(uint64_t) * l2_table_size, l2_table_offset) < 0)
-		goto out_error_free_l2;
+		goto out_error;
 
 	l2_idx		= get_l2_index(q, offset);
 
 	if (l2_idx >= l2_table_size)
-		goto out_error_free_l2;
+		goto out_error;
 
 	clust_start	= be64_to_cpu(l2_table[l2_idx]);
 
+	free(l2_table);
+
 	if (!clust_start)
 		goto zero_sector;
 
 	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;
-
-	free(l2_table);
+		goto out_error;
 
 	return 0;
 
@@ -95,9 +95,9 @@ zero_sector:
 
 	return 0;
 
-out_error_free_l2:
-	free(l2_table);
 out_error:
+	free(l2_table);
+
 	return -1;
 }
 
-- 
1.7.5.rc1

[PATCH 2/2] kvm tools: Add scatter-gather support for disk images

[Sasha Levin]

Add optional support for scatter-gather to disk_image.
Formats that can't take advantage of scatter-gather fallback to simple IO.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/disk-image.c             |   22 ++++++++++++++++++++
 tools/kvm/include/kvm/disk-image.h |   31 ++++++++++++++++++++++++++++
 tools/kvm/virtio-blk.c             |   39 +++++------------------------------
 3 files changed, 59 insertions(+), 33 deletions(-)

diff --git a/tools/kvm/disk-image.c b/tools/kvm/disk-image.c
index 4198ebb..dd54b99 100644
--- a/tools/kvm/disk-image.c
+++ b/tools/kvm/disk-image.c
@@ -72,6 +72,26 @@ static int raw_image__write_sector(struct disk_image *self, uint64_t sector, voi
 	return 0;
 }
 
+static size_t raw_image__read_sector_sg(struct disk_image *self, uint64_t sector, const struct iovec *iov, int iovcount)
+{
+	uint64_t offset = sector << SECTOR_SHIFT;
+	size_t ret = -1;
+
+	ret = preadv(self->fd, iov, iovcount, offset);
+
+	return ret;
+}
+
+static size_t raw_image__write_sector_sg(struct disk_image *self, uint64_t sector, const struct iovec *iov, int iovcount)
+{
+	uint64_t offset = sector << SECTOR_SHIFT;
+	size_t ret = -1;
+
+	ret = pwritev(self->fd, iov, iovcount, offset);
+
+	return ret;
+}
+
 static int raw_image__read_sector_ro_mmap(struct disk_image *self, uint64_t sector, void *dst, uint32_t dst_len)
 {
 	uint64_t offset = sector << SECTOR_SHIFT;
@@ -105,6 +125,8 @@ static void raw_image__close_ro_mmap(struct disk_image *self)
 static struct disk_image_operations raw_image_ops = {
 	.read_sector		= raw_image__read_sector,
 	.write_sector		= raw_image__write_sector,
+	.read_sector_sg		= raw_image__read_sector_sg,
+	.write_sector_sg	= raw_image__write_sector_sg
 };
 
 static struct disk_image_operations raw_image_ro_mmap_ops = {
diff --git a/tools/kvm/include/kvm/disk-image.h b/tools/kvm/include/kvm/disk-image.h
index 33962c6..a4a96a2 100644
--- a/tools/kvm/include/kvm/disk-image.h
+++ b/tools/kvm/include/kvm/disk-image.h
@@ -3,6 +3,7 @@
 
 #include <stdint.h>
 #include <stdbool.h>
+#include <sys/uio.h>
 
 #define SECTOR_SHIFT		9
 #define SECTOR_SIZE		(1UL << SECTOR_SHIFT)
@@ -12,6 +13,8 @@ struct disk_image;
 struct disk_image_operations {
 	int (*read_sector)(struct disk_image *self, uint64_t sector, void *dst, uint32_t dst_len);
 	int (*write_sector)(struct disk_image *self, uint64_t sector, void *src, uint32_t src_len);
+	size_t (*read_sector_sg)(struct disk_image *self, uint64_t sector, const struct iovec *iov, int iovcount);
+	size_t (*write_sector_sg)(struct disk_image *self, uint64_t sector, const struct iovec *iov, int iovcount);
 	void (*close)(struct disk_image *self);
 };
 
@@ -37,4 +40,32 @@ static inline int disk_image__write_sector(struct disk_image *self, uint64_t sec
 	return self->ops->write_sector(self, sector, src, src_len);
 }
 
+static inline size_t disk_image__read_sector_sg(struct disk_image *self, uint64_t sector, const struct iovec *iov, int iovcount)
+{
+	if (self->ops->read_sector_sg)
+		return self->ops->read_sector_sg(self, sector, iov, iovcount);
+
+	while (iovcount--) {
+		self->ops->read_sector(self, sector, iov->iov_base, iov->iov_len);
+		sector += iov->iov_len >> SECTOR_SHIFT;
+		iov++;
+	}
+
+	return sector << SECTOR_SHIFT;
+}
+
+static inline size_t disk_image__write_sector_sg(struct disk_image *self, uint64_t sector, const struct iovec *iov, int iovcount)
+{
+	if (self->ops->write_sector_sg)
+		return self->ops->write_sector_sg(self, sector, iov, iovcount);
+
+	while (iovcount--) {
+		self->ops->write_sector(self, sector, iov->iov_base, iov->iov_len);
+		sector += iov->iov_len >> SECTOR_SHIFT;
+		iov++;
+	}
+
+	return sector << SECTOR_SHIFT;
+}
+
 #endif /* KVM__DISK_IMAGE_H */
diff --git a/tools/kvm/virtio-blk.c b/tools/kvm/virtio-blk.c
index cb344d08..aee3b73 100644
--- a/tools/kvm/virtio-blk.c
+++ b/tools/kvm/virtio-blk.c
@@ -115,50 +115,23 @@ static bool virtio_blk_do_io_request(struct kvm *self, struct virt_queue *queue)
 {
 	struct iovec iov[VIRTIO_BLK_QUEUE_SIZE];
 	struct virtio_blk_outhdr *req;
-	uint32_t block_len, block_cnt;
+	size_t block_cnt = -1;
 	uint16_t out, in, head;
 	uint8_t *status;
-	bool io_error;
-	void *block;
-	int err, i;
-
-	io_error	= false;
 
 	head		= virt_queue__get_iov(queue, iov, &out, &in, self);
 
 	/* head */
 	req		= iov[0].iov_base;
 
-	/* block */
-	block_cnt	= 0;
-
-	for (i = 1; i < out + in - 1; i++) {
-		block		= iov[i].iov_base;
-		block_len	= iov[i].iov_len;
-
-		switch (req->type) {
-		case VIRTIO_BLK_T_IN:
-			err	= disk_image__read_sector(self->disk_image, req->sector, block, block_len);
-			if (err)
-				io_error = true;
-			break;
-		case VIRTIO_BLK_T_OUT:
-			err	= disk_image__write_sector(self->disk_image, req->sector, block, block_len);
-			if (err)
-				io_error = true;
-			break;
-		default:
-			warning("request type %d", req->type);
-			io_error	= true;
-		}
-
-		req->sector	+= block_len >> SECTOR_SHIFT;
-		block_cnt	+= block_len;
-	}
+	if (req->type == VIRTIO_BLK_T_IN)
+		block_cnt = disk_image__read_sector_sg(self->disk_image, req->sector, iov + 1, in + out - 2);
+	else if (req->type == VIRTIO_BLK_T_OUT)
+		block_cnt = disk_image__write_sector_sg(self->disk_image, req->sector, iov + 1, in + out - 2);
 
 	/* status */
 	status			= iov[out + in - 1].iov_base;
-	*status			= io_error ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
+	*status			= (block_cnt == (size_t)-1) ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
 
 	virt_queue__set_used_elem(queue, head, block_cnt);
 
-- 
1.7.5.rc1

Re: [PATCH 2/2] kvm tools: Add scatter-gather support for disk images

[Pekka Enberg]

On Sat, 16 Apr 2011, Sasha Levin wrote:
> Add optional support for scatter-gather to disk_image.
> Formats that can't take advantage of scatter-gather fallback to simple IO.

Cool!

> +static size_t raw_image__read_sector_sg(struct disk_image *self, uint64_t sector, const struct iovec *iov, int iovcount)
> +{
> +	uint64_t offset = sector << SECTOR_SHIFT;
> +	size_t ret = -1;
> +
> +	ret = preadv(self->fd, iov, iovcount, offset);
> +
> +	return ret;

Please just do

   return preadv(...);

Also, I guess you need preadv_in_full() similar to read_in_full() that 
handles EINTR and EAGAIN.

> +}
> +
> +static size_t raw_image__write_sector_sg(struct disk_image *self, uint64_t sector, const struct iovec *iov, int iovcount)
> +{
> +	uint64_t offset = sector << SECTOR_SHIFT;
> +	size_t ret = -1;
> +
> +	ret = pwritev(self->fd, iov, iovcount, offset);
> +
> +	return ret;

Same comments apply here as well.

> @@ -115,50 +115,23 @@ static bool virtio_blk_do_io_request(struct kvm *self, struct virt_queue *queue)
> {
> 	struct iovec iov[VIRTIO_BLK_QUEUE_SIZE];
> 	struct virtio_blk_outhdr *req;
> -	uint32_t block_len, block_cnt;
> +	size_t block_cnt = -1;
> 	uint16_t out, in, head;
> 	uint8_t *status;
> -	bool io_error;
> -	void *block;
> -	int err, i;
> -
> -	io_error	= false;
>
> 	head		= virt_queue__get_iov(queue, iov, &out, &in, self);
>
> 	/* head */
> 	req		= iov[0].iov_base;
>
> -	/* block */
> -	block_cnt	= 0;
> -
> -	for (i = 1; i < out + in - 1; i++) {
> -		block		= iov[i].iov_base;
> -		block_len	= iov[i].iov_len;
> -
> -		switch (req->type) {
> -		case VIRTIO_BLK_T_IN:
> -			err	= disk_image__read_sector(self->disk_image, req->sector, block, block_len);
> -			if (err)
> -				io_error = true;
> -			break;
> -		case VIRTIO_BLK_T_OUT:
> -			err	= disk_image__write_sector(self->disk_image, req->sector, block, block_len);
> -			if (err)
> -				io_error = true;
> -			break;
> -		default:
> -			warning("request type %d", req->type);
> -			io_error	= true;
> -		}
> -
> -		req->sector	+= block_len >> SECTOR_SHIFT;
> -		block_cnt	+= block_len;
> -	}
> +	if (req->type == VIRTIO_BLK_T_IN)
> +		block_cnt = disk_image__read_sector_sg(self->disk_image, req->sector, iov + 1, in + out - 2);
> +	else if (req->type == VIRTIO_BLK_T_OUT)
> +		block_cnt = disk_image__write_sector_sg(self->disk_image, req->sector, iov + 1, in + out - 2);

Please us switch statement like in the above loop instead of if-else and 
retain the warning.

>
> 	/* status */
> 	status			= iov[out + in - 1].iov_base;
> -	*status			= io_error ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
> +	*status			= (block_cnt == (size_t)-1) ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
>
> 	virt_queue__set_used_elem(queue, head, block_cnt);
>
> -- 
> 1.7.5.rc1
>
>