[RFC 10/12] kvm tools: Fixes for disk image module

[Sasha Levin <levinsasha928@gmail.com>]

Fixes include:
 - Error handling
 - Cleanup
 - Standard init/uninit

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/builtin-run.c            |   27 +++++++++---
 tools/kvm/disk/blk.c               |   13 ++++--
 tools/kvm/disk/core.c              |   76 ++++++++++++++++++++++--------------
 tools/kvm/disk/qcow.c              |    2 +-
 tools/kvm/disk/raw.c               |    9 ++--
 tools/kvm/include/kvm/disk-image.h |    2 +-
 tools/kvm/include/kvm/virtio-blk.h |    5 +-
 tools/kvm/virtio/blk.c             |   37 +++++++++++++-----
 8 files changed, 111 insertions(+), 60 deletions(-)

diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
index 2950ea8..0b94ef0 100644
--- a/tools/kvm/builtin-run.c
+++ b/tools/kvm/builtin-run.c
@@ -1052,11 +1052,13 @@ static int kvm_cmd_run_init(int argc, const char **argv)
 
 	if (image_count) {
 		kvm->nr_disks = image_count;
-		kvm->disks    = disk_image__open_all(image_filename, readonly_image, image_count);
-		if (!kvm->disks)
-			die("Unable to load all disk images.");
-
-		virtio_blk__init_all(kvm);
+		kvm->disks = disk_image__open_all(image_filename, readonly_image, image_count);
+		if (IS_ERR(kvm->disks)) {
+			r = PTR_ERR(kvm->disks);
+			pr_error("disk_image__open_all() failed with error %ld\n",
+					PTR_ERR(kvm->disks));
+			goto fail;
+		}
 	}
 
 	printf("  # kvm run -k %s -m %Lu -c %d --name %s\n", kernel_filename, ram_size / 1024 / 1024, nrcpus, guest_name);
@@ -1082,6 +1084,12 @@ static int kvm_cmd_run_init(int argc, const char **argv)
 		goto fail;
 	}
 
+	r = virtio_blk__init(kvm);
+	if (r < 0) {
+		pr_error("virtio_blk__init() failed with error %d\n", r);
+		goto fail;
+	}
+
 	if (active_console == CONSOLE_VIRTIO)
 		virtio_console__init(kvm);
 
@@ -1222,10 +1230,15 @@ static void kvm_cmd_run_uninit(int guest_ret)
 
 	fb__stop();
 
-	virtio_blk__delete_all(kvm);
+	r = virtio_blk__uninit(kvm);
+	if (r < 0)
+		pr_warning("virtio_blk__uninit() failed with error %d\n", r);
+
 	virtio_rng__delete_all(kvm);
 
-	disk_image__close_all(kvm->disks, image_count);
+	r = disk_image__close_all(kvm->disks, image_count);
+	if (r < 0)
+		pr_warning("disk_image__close_all() failed with error %d\n", r);
 
 	r = serial8250__uninit(kvm);
 	if (r < 0)
diff --git a/tools/kvm/disk/blk.c b/tools/kvm/disk/blk.c
index 59294e8..72c1722 100644
--- a/tools/kvm/disk/blk.c
+++ b/tools/kvm/disk/blk.c
@@ -1,5 +1,7 @@
 #include "kvm/disk-image.h"
 
+#include <linux/err.h>
+
 /*
  * raw image and blk dev are similar, so reuse raw image ops.
  */
@@ -12,22 +14,23 @@ static struct disk_image_operations blk_dev_ops = {
 struct disk_image *blkdev__probe(const char *filename, struct stat *st)
 {
 	u64 size;
-	int fd;
+	int fd, r;
 
 	if (!S_ISBLK(st->st_mode))
-		return NULL;
+		return ERR_PTR(-EINVAL);
 
 	/*
 	 * Be careful! We are opening host block device!
 	 * Open it readonly since we do not want to break user's data on disk.
 	 */
-	fd			= open(filename, O_RDONLY);
+	fd = open(filename, O_RDONLY);
 	if (fd < 0)
-		return NULL;
+		return ERR_PTR(fd);
 
 	if (ioctl(fd, BLKGETSIZE64, &size) < 0) {
+		r = -errno;
 		close(fd);
-		return NULL;
+		return ERR_PTR(r);
 	}
 
 	/*
diff --git a/tools/kvm/disk/core.c b/tools/kvm/disk/core.c
index 4915efd..fb547ba 100644
--- a/tools/kvm/disk/core.c
+++ b/tools/kvm/disk/core.c
@@ -2,6 +2,7 @@
 #include "kvm/qcow.h"
 #include "kvm/virtio-blk.h"
 
+#include <linux/err.h>
 #include <sys/eventfd.h>
 #include <sys/poll.h>
 
@@ -31,14 +32,17 @@ static void *disk_image__thread(void *param)
 struct disk_image *disk_image__new(int fd, u64 size, struct disk_image_operations *ops, int use_mmap)
 {
 	struct disk_image *disk;
+	int r;
 
-	disk		= malloc(sizeof *disk);
+	disk = malloc(sizeof *disk);
 	if (!disk)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
-	disk->fd	= fd;
-	disk->size	= size;
-	disk->ops	= ops;
+	*disk = (struct disk_image) {
+		.fd	= fd,
+		.size	= size,
+		.ops	= ops,
+	};
 
 	if (use_mmap == DISK_IMAGE_MMAP) {
 		/*
@@ -46,8 +50,9 @@ struct disk_image *disk_image__new(int fd, u64 size, struct disk_image_operation
 		 */
 		disk->priv = mmap(NULL, size, PROT_RW, MAP_PRIVATE | MAP_NORESERVE, fd, 0);
 		if (disk->priv == MAP_FAILED) {
+			r = -errno;
 			free(disk);
-			disk = NULL;
+			return ERR_PTR(r);
 		}
 	}
 
@@ -57,8 +62,12 @@ struct disk_image *disk_image__new(int fd, u64 size, struct disk_image_operation
 
 		disk->evt = eventfd(0, 0);
 		io_setup(AIO_MAX, &disk->ctx);
-		if (pthread_create(&thread, NULL, disk_image__thread, disk) != 0)
-			die("Failed starting IO thread");
+		r = pthread_create(&thread, NULL, disk_image__thread, disk);
+		if (r) {
+			r = -errno;
+			free(disk);
+			return ERR_PTR(r);
+		}
 	}
 #endif
 	return disk;
@@ -71,54 +80,58 @@ struct disk_image *disk_image__open(const char *filename, bool readonly)
 	int fd;
 
 	if (stat(filename, &st) < 0)
-		return NULL;
+		return ERR_PTR(-errno);
 
 	/* blk device ?*/
-	disk		= blkdev__probe(filename, &st);
-	if (disk)
+	disk = blkdev__probe(filename, &st);
+	if (!IS_ERR_OR_NULL(disk))
 		return disk;
 
-	fd		= open(filename, readonly ? O_RDONLY : O_RDWR);
+	fd = open(filename, readonly ? O_RDONLY : O_RDWR);
 	if (fd < 0)
-		return NULL;
+		return ERR_PTR(fd);
 
 	/* qcow image ?*/
-	disk		= qcow_probe(fd, true);
-	if (disk) {
+	disk = qcow_probe(fd, true);
+	if (!IS_ERR_OR_NULL(disk)) {
 		pr_warning("Forcing read-only support for QCOW");
 		return disk;
 	}
 
 	/* raw image ?*/
-	disk		= raw_image__probe(fd, &st, readonly);
-	if (disk)
+	disk = raw_image__probe(fd, &st, readonly);
+	if (!IS_ERR_OR_NULL(disk))
 		return disk;
 
 	if (close(fd) < 0)
 		pr_warning("close() failed");
 
-	return NULL;
+	return ERR_PTR(-ENOSYS);
 }
 
 struct disk_image **disk_image__open_all(const char **filenames, bool *readonly, int count)
 {
 	struct disk_image **disks;
 	int i;
+	void *err;
 
-	if (!count || count > MAX_DISK_IMAGES)
-		return NULL;
+	if (!count)
+		return ERR_PTR(-EINVAL);
+	if (count > MAX_DISK_IMAGES)
+		return ERR_PTR(-ENOSPC);
 
 	disks = calloc(count, sizeof(*disks));
 	if (!disks)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	for (i = 0; i < count; i++) {
 		if (!filenames[i])
 			continue;
 
 		disks[i] = disk_image__open(filenames[i], readonly[i]);
-		if (!disks[i]) {
+		if (IS_ERR_OR_NULL(disks[i])) {
 			pr_error("Loading disk image '%s' failed", filenames[i]);
+			err = disks[i];
 			goto error;
 		}
 	}
@@ -126,10 +139,11 @@ struct disk_image **disk_image__open_all(const char **filenames, bool *readonly,
 	return disks;
 error:
 	for (i = 0; i < count; i++)
-		disk_image__close(disks[i]);
+		if (!IS_ERR_OR_NULL(disks[i]))
+			disk_image__close(disks[i]);
 
 	free(disks);
-	return NULL;
+	return err;
 }
 
 int disk_image__flush(struct disk_image *disk)
@@ -157,12 +171,14 @@ int disk_image__close(struct disk_image *disk)
 	return 0;
 }
 
-void disk_image__close_all(struct disk_image **disks, int count)
+int disk_image__close_all(struct disk_image **disks, int count)
 {
 	while (count)
 		disk_image__close(disks[--count]);
 
 	free(disks);
+
+	return 0;
 }
 
 /*
@@ -181,7 +197,7 @@ ssize_t disk_image__read(struct disk_image *disk, u64 sector, const struct iovec
 		total = disk->ops->read_sector(disk, sector, iov, iovcount, param);
 		if (total < 0) {
 			pr_info("disk_image__read error: total=%ld\n", (long)total);
-			return -1;
+			return total;
 		}
 	} else {
 		/* Do nothing */
@@ -213,7 +229,7 @@ ssize_t disk_image__write(struct disk_image *disk, u64 sector, const struct iove
 		total = disk->ops->write_sector(disk, sector, iov, iovcount, param);
 		if (total < 0) {
 			pr_info("disk_image__write error: total=%ld\n", (long)total);
-			return -1;
+			return total;
 		}
 	} else {
 		/* Do nothing */
@@ -228,9 +244,11 @@ ssize_t disk_image__write(struct disk_image *disk, u64 sector, const struct iove
 ssize_t disk_image__get_serial(struct disk_image *disk, void *buffer, ssize_t *len)
 {
 	struct stat st;
+	int r;
 
-	if (fstat(disk->fd, &st) != 0)
-		return 0;
+	r = fstat(disk->fd, &st);
+	if (r)
+		return r;
 
 	*len = snprintf(buffer, *len, "%llu%llu%llu", (u64)st.st_dev, (u64)st.st_rdev, (u64)st.st_ino);
 	return *len;
diff --git a/tools/kvm/disk/qcow.c b/tools/kvm/disk/qcow.c
index 712f811..23f11f2 100644
--- a/tools/kvm/disk/qcow.c
+++ b/tools/kvm/disk/qcow.c
@@ -1334,7 +1334,7 @@ static struct disk_image *qcow2_probe(int fd, bool readonly)
 	else
 		disk_image = disk_image__new(fd, h->size, &qcow_disk_ops, DISK_IMAGE_REGULAR);
 
-	if (!disk_image)
+	if (IS_ERR_OR_NULL(disk_image))
 		goto free_refcount_table;
 
 	disk_image->async = 0;
diff --git a/tools/kvm/disk/raw.c b/tools/kvm/disk/raw.c
index caa023c..d2df814 100644
--- a/tools/kvm/disk/raw.c
+++ b/tools/kvm/disk/raw.c
@@ -1,5 +1,7 @@
 #include "kvm/disk-image.h"
 
+#include <linux/err.h>
+
 #ifdef CONFIG_HAS_AIO
 #include <libaio.h>
 #endif
@@ -116,11 +118,10 @@ struct disk_image *raw_image__probe(int fd, struct stat *st, bool readonly)
 		struct disk_image *disk;
 
 		disk = disk_image__new(fd, st->st_size, &ro_ops, DISK_IMAGE_MMAP);
-		if (disk == NULL) {
-
+		if (IS_ERR_OR_NULL(disk)) {
 			disk = disk_image__new(fd, st->st_size, &ro_ops_nowrite, DISK_IMAGE_REGULAR);
 #ifdef CONFIG_HAS_AIO
-			if (disk)
+			if (!IS_ERR_OR_NULL(disk))
 				disk->async = 1;
 #endif
 		}
@@ -132,7 +133,7 @@ struct disk_image *raw_image__probe(int fd, struct stat *st, bool readonly)
 		 */
 		disk = disk_image__new(fd, st->st_size, &raw_image_regular_ops, DISK_IMAGE_REGULAR);
 #ifdef CONFIG_HAS_AIO
-		if (disk)
+		if (!IS_ERR_OR_NULL(disk))
 			disk->async = 1;
 #endif
 		return disk;
diff --git a/tools/kvm/include/kvm/disk-image.h b/tools/kvm/include/kvm/disk-image.h
index 56c08da..a0b61bf 100644
--- a/tools/kvm/include/kvm/disk-image.h
+++ b/tools/kvm/include/kvm/disk-image.h
@@ -57,7 +57,7 @@ struct disk_image *disk_image__open(const char *filename, bool readonly);
 struct disk_image **disk_image__open_all(const char **filenames, bool *readonly, int count);
 struct disk_image *disk_image__new(int fd, u64 size, struct disk_image_operations *ops, int mmap);
 int disk_image__close(struct disk_image *disk);
-void disk_image__close_all(struct disk_image **disks, int count);
+int disk_image__close_all(struct disk_image **disks, int count);
 int disk_image__flush(struct disk_image *disk);
 ssize_t disk_image__read(struct disk_image *disk, u64 sector, const struct iovec *iov,
 				int iovcount, void *param);
diff --git a/tools/kvm/include/kvm/virtio-blk.h b/tools/kvm/include/kvm/virtio-blk.h
index 63731a9..e0c0919 100644
--- a/tools/kvm/include/kvm/virtio-blk.h
+++ b/tools/kvm/include/kvm/virtio-blk.h
@@ -5,9 +5,8 @@
 
 struct kvm;
 
-void virtio_blk__init(struct kvm *kvm, struct disk_image *disk);
-void virtio_blk__init_all(struct kvm *kvm);
-void virtio_blk__delete_all(struct kvm *kvm);
+int virtio_blk__init(struct kvm *kvm);
+int virtio_blk__uninit(struct kvm *kvm);
 void virtio_blk_complete(void *param, long len);
 
 #endif /* KVM__BLK_VIRTIO_H */
diff --git a/tools/kvm/virtio/blk.c b/tools/kvm/virtio/blk.c
index d1a0197..e9b836a 100644
--- a/tools/kvm/virtio/blk.c
+++ b/tools/kvm/virtio/blk.c
@@ -208,17 +208,17 @@ static struct virtio_ops blk_dev_virtio_ops = (struct virtio_ops) {
 	.get_size_vq		= get_size_vq,
 };
 
-void virtio_blk__init(struct kvm *kvm, struct disk_image *disk)
+static int virtio_blk__init_one(struct kvm *kvm, struct disk_image *disk)
 {
 	struct blk_dev *bdev;
 	unsigned int i;
 
 	if (!disk)
-		return;
+		return -EINVAL;
 
 	bdev = calloc(1, sizeof(struct blk_dev));
 	if (bdev == NULL)
-		die("Failed allocating bdev");
+		return -ENOMEM;
 
 	*bdev = (struct blk_dev) {
 		.mutex			= PTHREAD_MUTEX_INITIALIZER,
@@ -251,23 +251,40 @@ void virtio_blk__init(struct kvm *kvm, struct disk_image *disk)
 						"Please make sure that the guest kernel was "
 						"compiled with CONFIG_VIRTIO_BLK=y enabled "
 						"in its .config");
+	return 0;
 }
 
-void virtio_blk__init_all(struct kvm *kvm)
+static int virtio_blk__uninit_one(struct kvm *kvm, struct blk_dev *bdev)
 {
-	int i;
+	list_del(&bdev->list);
+	free(bdev);
 
-	for (i = 0; i < kvm->nr_disks; i++)
-		virtio_blk__init(kvm, kvm->disks[i]);
+	return 0;
 }
 
-void virtio_blk__delete_all(struct kvm *kvm)
+int virtio_blk__init(struct kvm *kvm)
+{
+	int i, r = 0;
+
+	for (i = 0; i < kvm->nr_disks; i++) {
+		r = virtio_blk__init_one(kvm, kvm->disks[i]);
+		if (r < 0)
+			goto cleanup;
+	}
+
+	return 0;
+cleanup:
+	return virtio_blk__uninit(kvm);
+}
+
+int virtio_blk__uninit(struct kvm *kvm)
 {
 	while (!list_empty(&bdevs)) {
 		struct blk_dev *bdev;
 
 		bdev = list_first_entry(&bdevs, struct blk_dev, list);
-		list_del(&bdev->list);
-		free(bdev);
+		virtio_blk__uninit_one(kvm, bdev);
 	}
+
+	return 0;
 }
-- 
1.7.8