From 54db38a47978381e23e7f6479c31a97b5d352f7e Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 14 Apr 2014 14:47:14 +0200 Subject: block: Fix nb_sectors check in bdrv_check_byte_request() nb_sectors is signed, check for negative values. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block.c b/block.c index 990a7542a9..3b7951eb4f 100644 --- a/block.c +++ b/block.c @@ -2601,7 +2601,7 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset, static int bdrv_check_request(BlockDriverState *bs, int64_t sector_num, int nb_sectors) { - if (nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) { + if (nb_sectors < 0 || nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) { return -EIO; } -- cgit v1.2.1 From 1dd3a44753f10970ded50950d28353c00bfcaf91 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 14 Apr 2014 14:48:16 +0200 Subject: block: Limit size to INT_MAX in bdrv_check_byte_request() Commit 8f4754ed intended to protect against integer overflow bugs in block drivers by making sure that a single request that is passed to drivers is no longer than INT_MAX bytes. However, meanwhile there are some callers that don't use that code path any more but call bdrv_check_byte_request() directy, so let's add a check there as well. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/block.c b/block.c index 3b7951eb4f..5a0b421655 100644 --- a/block.c +++ b/block.c @@ -2581,6 +2581,10 @@ static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset, { int64_t len; + if (size > INT_MAX) { + return -EIO; + } + if (!bdrv_is_inserted(bs)) return -ENOMEDIUM; -- cgit v1.2.1 From da15ee5134f715adb07e3688a1c6e8b42cb6ac94 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 14 Apr 2014 15:39:36 +0200 Subject: block: Catch integer overflow in bdrv_rw_co() Insanely large requests could cause an integer overflow in bdrv_rw_co() while converting sectors to bytes. This patch catches the problem and returns an error (if we hadn't overflown the integer here, bdrv_check_byte_request() would have rejected the request, so we're not breaking anything that was supposed to work before). We actually do have a test case that triggers behaviour where we accidentally let such a request pass, so that it would return success, but read 0 bytes instead of the requested 4 GB. It fails now like it should. If the vdi block driver wants to be able to deal with huge images, it can't read the whole block bitmap at once into memory like it does today, but needs to use a metadata cache like qcow2 does. Signed-off-by: Kevin Wolf --- block.c | 4 ++++ tests/qemu-iotests/084.out | 5 +---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/block.c b/block.c index 5a0b421655..ec3fa503df 100644 --- a/block.c +++ b/block.c @@ -2690,6 +2690,10 @@ static int bdrv_rw_co(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, .iov_len = nb_sectors * BDRV_SECTOR_SIZE, }; + if (nb_sectors < 0 || nb_sectors > INT_MAX / BDRV_SECTOR_SIZE) { + return -EINVAL; + } + qemu_iovec_init_external(&qiov, &iov, 1); return bdrv_prwv_co(bs, sector_num << BDRV_SECTOR_BITS, &qiov, is_write, flags); diff --git a/tests/qemu-iotests/084.out b/tests/qemu-iotests/084.out index e681924b85..c7120d9b0b 100644 --- a/tests/qemu-iotests/084.out +++ b/tests/qemu-iotests/084.out @@ -4,10 +4,7 @@ QA output created by 084 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 Test 1: Maximum size (1024 TB): -image: TEST_DIR/t.IMGFMT -file format: IMGFMT -virtual size: 1024T (1125899905794048 bytes) -cluster_size: 1048576 +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Could not open 'TEST_DIR/t.IMGFMT': Invalid argument Test 2: Size too large (1024TB + 1) qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported VDI image size (size is 0x3fffffff10000, max supported is 0x3fffffff00000) -- cgit v1.2.1 From 9ce10c0bdcdd8a36c62e3376fd1de86bc0fb8a2a Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 14 Apr 2014 17:03:34 +0200 Subject: block: Check bdrv_getlength() return value in bdrv_make_zero() Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/block.c b/block.c index ec3fa503df..da558774e8 100644 --- a/block.c +++ b/block.c @@ -2749,10 +2749,16 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num, */ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags) { - int64_t target_size = bdrv_getlength(bs) / BDRV_SECTOR_SIZE; + int64_t target_size; int64_t ret, nb_sectors, sector_num = 0; int n; + target_size = bdrv_getlength(bs); + if (target_size < 0) { + return target_size; + } + target_size /= BDRV_SECTOR_SIZE; + for (;;) { nb_sectors = target_size - sector_num; if (nb_sectors <= 0) { -- cgit v1.2.1 From 4ab9dab5b9be0381e714d3fbe518689a72459011 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Thu, 17 Apr 2014 11:34:37 +0800 Subject: vmdk: Fix %d and %lld to PRI* in format strings Signed-off-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/vmdk.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index b69988d169..938a183e66 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -640,7 +640,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, if (le32_to_cpu(header.version) > 3) { char buf[64]; - snprintf(buf, sizeof(buf), "VMDK version %d", + snprintf(buf, sizeof(buf), "VMDK version %" PRId32, le32_to_cpu(header.version)); error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, bs->device_name, "vmdk", buf); @@ -671,8 +671,9 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, } if (bdrv_getlength(file) < le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE) { - error_setg(errp, "File truncated, expecting at least %lld bytes", - le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE); + error_setg(errp, "File truncated, expecting at least %" PRId64 " bytes", + (int64_t)(le64_to_cpu(header.grain_offset) + * BDRV_SECTOR_SIZE)); return -EINVAL; } @@ -1720,7 +1721,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options, "\n" "ddb.virtualHWVersion = \"%d\"\n" "ddb.geometry.cylinders = \"%" PRId64 "\"\n" - "ddb.geometry.heads = \"%d\"\n" + "ddb.geometry.heads = \"%" PRIu32 "\"\n" "ddb.geometry.sectors = \"63\"\n" "ddb.adapterType = \"%s\"\n"; @@ -1780,9 +1781,9 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options, strcmp(fmt, "twoGbMaxExtentFlat")); compress = !strcmp(fmt, "streamOptimized"); if (flat) { - desc_extent_line = "RW %lld FLAT \"%s\" 0\n"; + desc_extent_line = "RW %" PRId64 " FLAT \"%s\" 0\n"; } else { - desc_extent_line = "RW %lld SPARSE \"%s\"\n"; + desc_extent_line = "RW %" PRId64 " SPARSE \"%s\"\n"; } if (flat && backing_file) { error_setg(errp, "Flat image can't have backing file"); -- cgit v1.2.1 From b8afb520e479e693c227aa39c2fb7670743e104f Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Wed, 16 Apr 2014 09:34:30 +0800 Subject: block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap bdrv_getlength could fail, check the return value before using it. Return NULL and set errno if it fails. Callers are updated to handle the error case. Signed-off-by: Fam Zheng Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- block-migration.c | 30 ++++++++++++++++++++++++++---- block.c | 11 +++++++++-- block/mirror.c | 5 ++++- include/block/block.h | 3 ++- 4 files changed, 41 insertions(+), 8 deletions(-) diff --git a/block-migration.c b/block-migration.c index 897fdbabb5..56951e09ae 100644 --- a/block-migration.c +++ b/block-migration.c @@ -310,13 +310,28 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds) /* Called with iothread lock taken. */ -static void set_dirty_tracking(void) +static int set_dirty_tracking(void) { BlkMigDevState *bmds; + int ret; QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) { - bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE); + bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE, + NULL); + if (!bmds->dirty_bitmap) { + ret = -errno; + goto fail; + } } + return 0; + +fail: + QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) { + if (bmds->dirty_bitmap) { + bdrv_release_dirty_bitmap(bmds->bs, bmds->dirty_bitmap); + } + } + return ret; } static void unset_dirty_tracking(void) @@ -611,10 +626,17 @@ static int block_save_setup(QEMUFile *f, void *opaque) block_mig_state.submitted, block_mig_state.transferred); qemu_mutex_lock_iothread(); - init_blk_migration(f); /* start track dirty blocks */ - set_dirty_tracking(); + ret = set_dirty_tracking(); + + if (ret) { + qemu_mutex_unlock_iothread(); + return ret; + } + + init_blk_migration(f); + qemu_mutex_unlock_iothread(); ret = flush_blks(f); diff --git a/block.c b/block.c index da558774e8..0c81747268 100644 --- a/block.c +++ b/block.c @@ -5110,7 +5110,8 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov) return true; } -BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity) +BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity, + Error **errp) { int64_t bitmap_size; BdrvDirtyBitmap *bitmap; @@ -5119,7 +5120,13 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity) granularity >>= BDRV_SECTOR_BITS; assert(granularity); - bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS); + bitmap_size = bdrv_getlength(bs); + if (bitmap_size < 0) { + error_setg_errno(errp, -bitmap_size, "could not get length of device"); + errno = -bitmap_size; + return NULL; + } + bitmap_size >>= BDRV_SECTOR_BITS; bitmap = g_malloc0(sizeof(BdrvDirtyBitmap)); bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1); QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list); diff --git a/block/mirror.c b/block/mirror.c index 0ef41f999e..2618c3763c 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -605,7 +605,10 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, s->granularity = granularity; s->buf_size = MAX(buf_size, granularity); - s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity); + s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, errp); + if (!s->dirty_bitmap) { + return; + } bdrv_set_enable_write_cache(s->target, true); bdrv_set_on_error(s->target, on_target_error, on_target_error); bdrv_iostatus_enable(s->target); diff --git a/include/block/block.h b/include/block/block.h index b3230a25f6..2b51eec571 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -429,7 +429,8 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov); struct HBitmapIter; typedef struct BdrvDirtyBitmap BdrvDirtyBitmap; -BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity); +BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity, + Error **errp); void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs); int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector); -- cgit v1.2.1 From acd7fdc6d80711371d7a1507a22438d9465da63c Mon Sep 17 00:00:00 2001 From: Maria Kustova Date: Tue, 18 Mar 2014 09:59:18 +0400 Subject: curl: Replaced old error handling with error reporting API. Signed-off-by: Maria Kustova Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- block/curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/curl.c b/block/curl.c index 1b9b1f6341..6731d2891f 100644 --- a/block/curl.c +++ b/block/curl.c @@ -543,7 +543,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, int flags, return 0; out: - fprintf(stderr, "CURL: Error opening file: %s\n", state->errmsg); + error_setg(errp, "CURL: Error opening file: %s", state->errmsg); curl_easy_cleanup(state->curl); state->curl = NULL; out_noclean: -- cgit v1.2.1 From 636ea3708c253e9d2ddac6bd7d96854ba95fb7f5 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 24 Jan 2014 14:11:52 +0100 Subject: block: Remove -errno return value from bdrv_assign_node_name It takes an errp argument. That's enough for error handling. Signed-off-by: Kevin Wolf --- block.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/block.c b/block.c index 0c81747268..0ff5764cdb 100644 --- a/block.c +++ b/block.c @@ -788,38 +788,36 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags) return open_flags; } -static int bdrv_assign_node_name(BlockDriverState *bs, - const char *node_name, - Error **errp) +static void bdrv_assign_node_name(BlockDriverState *bs, + const char *node_name, + Error **errp) { if (!node_name) { - return 0; + return; } /* empty string node name is invalid */ if (node_name[0] == '\0') { error_setg(errp, "Empty node name"); - return -EINVAL; + return; } /* takes care of avoiding namespaces collisions */ if (bdrv_find(node_name)) { error_setg(errp, "node-name=%s is conflicting with a device id", node_name); - return -EINVAL; + return; } /* takes care of avoiding duplicates node names */ if (bdrv_find_node(node_name)) { error_setg(errp, "Duplicate node name"); - return -EINVAL; + return; } /* copy node name into the bs and insert it into the graph list */ pstrcpy(bs->node_name, sizeof(bs->node_name), node_name); QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list); - - return 0; } /* @@ -854,9 +852,10 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name); node_name = qdict_get_try_str(options, "node-name"); - ret = bdrv_assign_node_name(bs, node_name, errp); - if (ret < 0) { - return ret; + bdrv_assign_node_name(bs, node_name, &local_err); + if (error_is_set(&local_err)) { + error_propagate(errp, local_err); + return -EINVAL; } qdict_del(options, "node-name"); -- cgit v1.2.1 From 5ff679b47d151c4f73be297f96606eaefb6cc4c4 Mon Sep 17 00:00:00 2001 From: Aakriti Gupta Date: Sat, 15 Mar 2014 15:05:23 +0530 Subject: convert fprintf() calls to error_setg() in block/qed.c:bdrv_qed_create() This patch converts fprintf() calls to error_setg() in block/qed.c:bdrv_qed_create() (error_setg() is part of error reporting API in include/qapi/error.h) Signed-off-by: Aakriti Gupta Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- block/qed.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/block/qed.c b/block/qed.c index 3bd9db9c85..c130e42d0d 100644 --- a/block/qed.c +++ b/block/qed.c @@ -650,19 +650,21 @@ static int bdrv_qed_create(const char *filename, QEMUOptionParameter *options, } if (!qed_is_cluster_size_valid(cluster_size)) { - fprintf(stderr, "QED cluster size must be within range [%u, %u] and power of 2\n", - QED_MIN_CLUSTER_SIZE, QED_MAX_CLUSTER_SIZE); + error_setg(errp, "QED cluster size must be within range [%u, %u] " + "and power of 2", + QED_MIN_CLUSTER_SIZE, QED_MAX_CLUSTER_SIZE); return -EINVAL; } if (!qed_is_table_size_valid(table_size)) { - fprintf(stderr, "QED table size must be within range [%u, %u] and power of 2\n", - QED_MIN_TABLE_SIZE, QED_MAX_TABLE_SIZE); + error_setg(errp, "QED table size must be within range [%u, %u] " + "and power of 2", + QED_MIN_TABLE_SIZE, QED_MAX_TABLE_SIZE); return -EINVAL; } if (!qed_is_image_size_valid(image_size, cluster_size, table_size)) { - fprintf(stderr, "QED image size must be a non-zero multiple of " - "cluster size and less than %" PRIu64 " bytes\n", - qed_max_image_size(cluster_size, table_size)); + error_setg(errp, "QED image size must be a non-zero multiple of " + "cluster size and less than %" PRIu64 " bytes", + qed_max_image_size(cluster_size, table_size)); return -EINVAL; } -- cgit v1.2.1 From 98522f63f40adaebc412481e1d2e9170160d4539 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 17 Apr 2014 13:16:01 +0200 Subject: block: Add errp to bdrv_new() This patch adds an errp parameter to bdrv_new() and updates all its callers. The next patches will make use of this in order to check for duplicate IDs. Most of the callers know that their ID is fine, so they can simply assert that there is no error. Behaviour doesn't change with this patch yet as bdrv_new() doesn't actually assign errors to errp. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- block.c | 6 +++--- block/iscsi.c | 2 +- block/vvfat.c | 2 +- blockdev.c | 9 +++++++-- hw/block/xen_disk.c | 7 +++++-- include/block/block.h | 2 +- qemu-img.c | 6 +++--- qemu-io.c | 2 +- qemu-nbd.c | 3 ++- 9 files changed, 24 insertions(+), 15 deletions(-) diff --git a/block.c b/block.c index 0ff5764cdb..f3b93c942b 100644 --- a/block.c +++ b/block.c @@ -332,7 +332,7 @@ void bdrv_register(BlockDriver *bdrv) } /* create a new block device (by default it is empty) */ -BlockDriverState *bdrv_new(const char *device_name) +BlockDriverState *bdrv_new(const char *device_name, Error **errp) { BlockDriverState *bs; @@ -1220,7 +1220,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp) qdict_put(snapshot_options, "file.filename", qstring_from_str(tmp_filename)); - bs_snapshot = bdrv_new(""); + bs_snapshot = bdrv_new("", &error_abort); bs_snapshot->is_temporary = 1; ret = bdrv_open(&bs_snapshot, NULL, NULL, snapshot_options, @@ -1287,7 +1287,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, if (*pbs) { bs = *pbs; } else { - bs = bdrv_new(""); + bs = bdrv_new("", &error_abort); } /* NULL means an empty set of options */ diff --git a/block/iscsi.c b/block/iscsi.c index f425573df8..a636ea4f53 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -1401,7 +1401,7 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options, IscsiLun *iscsilun = NULL; QDict *bs_options; - bs = bdrv_new(""); + bs = bdrv_new("", &error_abort); /* Read out options */ while (options && options->name) { diff --git a/block/vvfat.c b/block/vvfat.c index 1978c9ed62..c3af7ff4c5 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -2947,7 +2947,7 @@ static int enable_write_target(BDRVVVFATState *s) unlink(s->qcow_filename); #endif - s->bs->backing_hd = bdrv_new(""); + s->bs->backing_hd = bdrv_new("", &error_abort); s->bs->backing_hd->drv = &vvfat_write_target; s->bs->backing_hd->opaque = g_malloc(sizeof(void*)); *(void**)s->bs->backing_hd->opaque = s; diff --git a/blockdev.c b/blockdev.c index 5dd01ea147..3a11a62019 100644 --- a/blockdev.c +++ b/blockdev.c @@ -461,7 +461,11 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, /* init */ dinfo = g_malloc0(sizeof(*dinfo)); dinfo->id = g_strdup(qemu_opts_id(opts)); - dinfo->bdrv = bdrv_new(dinfo->id); + dinfo->bdrv = bdrv_new(dinfo->id, &error); + if (error) { + error_propagate(errp, error); + goto bdrv_new_err; + } dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0; dinfo->bdrv->read_only = ro; dinfo->refcount = 1; @@ -523,8 +527,9 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, err: bdrv_unref(dinfo->bdrv); - g_free(dinfo->id); QTAILQ_REMOVE(&drives, dinfo, next); +bdrv_new_err: + g_free(dinfo->id); g_free(dinfo); early_err: QDECREF(bs_opts); diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c index bc061e6403..a8fea72edf 100644 --- a/hw/block/xen_disk.c +++ b/hw/block/xen_disk.c @@ -817,11 +817,14 @@ static int blk_connect(struct XenDevice *xendev) index = (blkdev->xendev.dev - 202 * 256) / 16; blkdev->dinfo = drive_get(IF_XEN, 0, index); if (!blkdev->dinfo) { + Error *local_err = NULL; /* setup via xenbus -> create new block driver instance */ xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n"); - blkdev->bs = bdrv_new(blkdev->dev); + blkdev->bs = bdrv_new(blkdev->dev, &local_err); + if (local_err) { + blkdev->bs = NULL; + } if (blkdev->bs) { - Error *local_err = NULL; BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto, readonly); if (bdrv_open(&blkdev->bs, blkdev->filename, NULL, NULL, qflags, diff --git a/include/block/block.h b/include/block/block.h index 2b51eec571..c12808a252 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -180,7 +180,7 @@ int bdrv_create(BlockDriver *drv, const char* filename, QEMUOptionParameter *options, Error **errp); int bdrv_create_file(const char* filename, QEMUOptionParameter *options, Error **errp); -BlockDriverState *bdrv_new(const char *device_name); +BlockDriverState *bdrv_new(const char *device_name, Error **errp); void bdrv_make_anon(BlockDriverState *bs); void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old); void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top); diff --git a/qemu-img.c b/qemu-img.c index 8455994c65..3e8bd800ff 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -274,7 +274,7 @@ static BlockDriverState *bdrv_new_open(const char *filename, Error *local_err = NULL; int ret; - bs = bdrv_new("image"); + bs = bdrv_new("image", &error_abort); if (fmt) { drv = bdrv_find_format(fmt); @@ -2344,7 +2344,7 @@ static int img_rebase(int argc, char **argv) } else { char backing_name[1024]; - bs_old_backing = bdrv_new("old_backing"); + bs_old_backing = bdrv_new("old_backing", &error_abort); bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name)); ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, BDRV_O_FLAGS, old_backing_drv, &local_err); @@ -2355,7 +2355,7 @@ static int img_rebase(int argc, char **argv) goto out; } if (out_baseimg[0]) { - bs_new_backing = bdrv_new("new_backing"); + bs_new_backing = bdrv_new("new_backing", &error_abort); ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL, BDRV_O_FLAGS, new_backing_drv, &local_err); if (ret) { diff --git a/qemu-io.c b/qemu-io.c index 5d7b53f756..9fcd72bb10 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -67,7 +67,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts) return 1; } } else { - qemuio_bs = bdrv_new("hda"); + qemuio_bs = bdrv_new("hda", &error_abort); if (bdrv_open(&qemuio_bs, name, NULL, opts, flags, NULL, &local_err) < 0) diff --git a/qemu-nbd.c b/qemu-nbd.c index 899e67cfd7..eed79fa15e 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -657,7 +657,8 @@ int main(int argc, char **argv) drv = NULL; } - bs = bdrv_new("hda"); + bs = bdrv_new("hda", &error_abort); + srcpath = argv[optind]; ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err); if (ret < 0) { -- cgit v1.2.1 From 9ffe333276de8ef463896303fb951f03fd4ffcb4 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 17 Apr 2014 16:57:13 +0200 Subject: qemu-img: Avoid duplicate block device IDs qemu-img used to use "image" as ID for all block devices. This means that e.g. img_convert() ended up with potentially multiple source images and one target image, all with the same ID. The next patch will catch this and fail to open the block device. This patch makes sure that qemu-img uses meaningful unique IDs for the block devices it uses. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- qemu-img.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index 3e8bd800ff..fb626aceb4 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -262,7 +262,8 @@ static int print_block_option_help(const char *filename, const char *fmt) return 0; } -static BlockDriverState *bdrv_new_open(const char *filename, +static BlockDriverState *bdrv_new_open(const char *id, + const char *filename, const char *fmt, int flags, bool require_io, @@ -274,7 +275,7 @@ static BlockDriverState *bdrv_new_open(const char *filename, Error *local_err = NULL; int ret; - bs = bdrv_new("image", &error_abort); + bs = bdrv_new(id, &error_abort); if (fmt) { drv = bdrv_find_format(fmt); @@ -615,7 +616,7 @@ static int img_check(int argc, char **argv) return 1; } - bs = bdrv_new_open(filename, fmt, flags, true, quiet); + bs = bdrv_new_open("image", filename, fmt, flags, true, quiet); if (!bs) { return 1; } @@ -724,7 +725,7 @@ static int img_commit(int argc, char **argv) return -1; } - bs = bdrv_new_open(filename, fmt, flags, true, quiet); + bs = bdrv_new_open("image", filename, fmt, flags, true, quiet); if (!bs) { return 1; } @@ -967,14 +968,14 @@ static int img_compare(int argc, char **argv) /* Initialize before goto out */ qemu_progress_init(progress, 2.0); - bs1 = bdrv_new_open(filename1, fmt1, BDRV_O_FLAGS, true, quiet); + bs1 = bdrv_new_open("image 1", filename1, fmt1, BDRV_O_FLAGS, true, quiet); if (!bs1) { error_report("Can't open file %s", filename1); ret = 2; goto out3; } - bs2 = bdrv_new_open(filename2, fmt2, BDRV_O_FLAGS, true, quiet); + bs2 = bdrv_new_open("image 2", filename2, fmt2, BDRV_O_FLAGS, true, quiet); if (!bs2) { error_report("Can't open file %s", filename2); ret = 2; @@ -1292,8 +1293,11 @@ static int img_convert(int argc, char **argv) total_sectors = 0; for (bs_i = 0; bs_i < bs_n; bs_i++) { - bs[bs_i] = bdrv_new_open(argv[optind + bs_i], fmt, BDRV_O_FLAGS, true, - quiet); + char *id = bs_n > 1 ? g_strdup_printf("source %d", bs_i) + : g_strdup("source"); + bs[bs_i] = bdrv_new_open(id, argv[optind + bs_i], fmt, BDRV_O_FLAGS, + true, quiet); + g_free(id); if (!bs[bs_i]) { error_report("Could not open '%s'", argv[optind + bs_i]); ret = -1; @@ -1416,7 +1420,7 @@ static int img_convert(int argc, char **argv) return -1; } - out_bs = bdrv_new_open(out_filename, out_fmt, flags, true, quiet); + out_bs = bdrv_new_open("target", out_filename, out_fmt, flags, true, quiet); if (!out_bs) { ret = -1; goto out; @@ -1799,8 +1803,8 @@ static ImageInfoList *collect_image_info_list(const char *filename, } g_hash_table_insert(filenames, (gpointer)filename, NULL); - bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING, - false, false); + bs = bdrv_new_open("image", filename, fmt, + BDRV_O_FLAGS | BDRV_O_NO_BACKING, false, false); if (!bs) { goto err; } @@ -2060,7 +2064,7 @@ static int img_map(int argc, char **argv) return 1; } - bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS, true, false); + bs = bdrv_new_open("image", filename, fmt, BDRV_O_FLAGS, true, false); if (!bs) { return 1; } @@ -2180,7 +2184,7 @@ static int img_snapshot(int argc, char **argv) filename = argv[optind++]; /* Open the image */ - bs = bdrv_new_open(filename, NULL, bdrv_oflags, true, quiet); + bs = bdrv_new_open("image", filename, NULL, bdrv_oflags, true, quiet); if (!bs) { return 1; } @@ -2309,7 +2313,7 @@ static int img_rebase(int argc, char **argv) * Ignore the old backing file for unsafe rebase in case we want to correct * the reference to a renamed or moved backing file. */ - bs = bdrv_new_open(filename, fmt, flags, true, quiet); + bs = bdrv_new_open("image", filename, fmt, flags, true, quiet); if (!bs) { return 1; } @@ -2606,7 +2610,8 @@ static int img_resize(int argc, char **argv) n = qemu_opt_get_size(param, BLOCK_OPT_SIZE, 0); qemu_opts_del(param); - bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR, true, quiet); + bs = bdrv_new_open("image", filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR, + true, quiet); if (!bs) { ret = -1; goto out; @@ -2707,7 +2712,8 @@ static int img_amend(int argc, char **argv) help(); } - bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR, true, quiet); + bs = bdrv_new_open("image", filename, fmt, + BDRV_O_FLAGS | BDRV_O_RDWR, true, quiet); if (!bs) { error_report("Could not open image '%s'", filename); ret = -1; -- cgit v1.2.1 From f2d953ec31eeeb3029ca915a55938c538a14efa8 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 17 Apr 2014 13:27:05 +0200 Subject: block: Catch duplicate IDs in bdrv_new() Since commit f298d071, block devices added with blockdev-add don't have a QemuOpts around in dinfo->opts. Consequently, we can't rely any more on QemuOpts catching duplicate IDs for block devices. This patch adds a new check for duplicate IDs to bdrv_new(), and moves the existing check that the ID isn't already taken for a node-name there as well. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake --- block.c | 11 +++++++++++ blockdev.c | 6 ------ tests/qemu-iotests/087 | 33 +++++++++++++++++++++++++++++++++ tests/qemu-iotests/087.out | 13 +++++++++++++ 4 files changed, 57 insertions(+), 6 deletions(-) diff --git a/block.c b/block.c index f3b93c942b..fc2edd33ae 100644 --- a/block.c +++ b/block.c @@ -336,6 +336,17 @@ BlockDriverState *bdrv_new(const char *device_name, Error **errp) { BlockDriverState *bs; + if (bdrv_find(device_name)) { + error_setg(errp, "Device with id '%s' already exists", + device_name); + return NULL; + } + if (bdrv_find_node(device_name)) { + error_setg(errp, "Device with node-name '%s' already exists", + device_name); + return NULL; + } + bs = g_malloc0(sizeof(BlockDriverState)); QLIST_INIT(&bs->dirty_bitmaps); pstrcpy(bs->device_name, sizeof(bs->device_name), device_name); diff --git a/blockdev.c b/blockdev.c index 3a11a62019..09826f10cf 100644 --- a/blockdev.c +++ b/blockdev.c @@ -452,12 +452,6 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, } } - if (bdrv_find_node(qemu_opts_id(opts))) { - error_setg(errp, "device id=%s is conflicting with a node-name", - qemu_opts_id(opts)); - goto early_err; - } - /* init */ dinfo = g_malloc0(sizeof(*dinfo)); dinfo->id = g_strdup(qemu_opts_id(opts)); diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087 index a38bb702b3..37d82fcdcd 100755 --- a/tests/qemu-iotests/087 +++ b/tests/qemu-iotests/087 @@ -72,6 +72,39 @@ run_qemu < Date: Thu, 17 Apr 2014 13:40:30 +0200 Subject: qemu-iotests: Check common namespace for id and node-name A name that is taken by an ID can't be taken by a node-name at the same time. Check that conflicts are correctly detected. Signed-off-by: Kevin Wolf Reviewed-by: Fam Zheng --- tests/qemu-iotests/087 | 52 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/087.out | 5 +++++ 2 files changed, 57 insertions(+) diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087 index 37d82fcdcd..82c56b1394 100755 --- a/tests/qemu-iotests/087 +++ b/tests/qemu-iotests/087 @@ -83,6 +83,7 @@ run_qemu < Date: Tue, 22 Apr 2014 13:36:11 +0800 Subject: qemu-img: Improve error messages Previously, when there is a user error in argv parsing, qemu-img prints help text and exits. Add an error_exit function to print a helpful error message and a hint to run 'qemu-img --help' for more information. As a bonus, "qemu-img --help" now has a more reasonable exit code 0. In the future the help text should be split by sub command, and only print the information for the specified command. Signed-off-by: Fam Zheng Signed-off-by: Kevin Wolf --- qemu-img.c | 74 ++++++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 46 insertions(+), 28 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index fb626aceb4..4dae84a182 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -57,8 +57,22 @@ static void format_print(void *opaque, const char *name) printf(" %s", name); } +static void QEMU_NORETURN GCC_FMT_ATTR(1, 2) error_exit(const char *fmt, ...) +{ + va_list ap; + + error_printf("qemu-img: "); + + va_start(ap, fmt); + error_vprintf(fmt, ap); + va_end(ap); + + error_printf("\nTry 'qemu-img --help' for more information\n"); + exit(EXIT_FAILURE); +} + /* Please keep in synch with qemu-img.texi */ -static void help(void) +static void QEMU_NORETURN help(void) { const char *help_msg = "qemu-img version " QEMU_VERSION ", Copyright (c) 2004-2008 Fabrice Bellard\n" @@ -129,7 +143,7 @@ static void help(void) printf("%s\nSupported formats:", help_msg); bdrv_iterate_format(format_print, NULL); printf("\n"); - exit(1); + exit(EXIT_SUCCESS); } static int GCC_FMT_ATTR(2, 3) qprintf(bool quiet, const char *fmt, ...) @@ -399,7 +413,7 @@ static int img_create(int argc, char **argv) } if (optind >= argc) { - help(); + error_exit("Expecting image file name"); } optind++; @@ -422,7 +436,7 @@ static int img_create(int argc, char **argv) img_size = (uint64_t)sval; } if (optind != argc) { - help(); + error_exit("Unexpected argument: %s", argv[optind]); } bdrv_img_create(filename, fmt, base_filename, base_fmt, @@ -591,7 +605,8 @@ static int img_check(int argc, char **argv) } else if (!strcmp(optarg, "all")) { fix = BDRV_FIX_LEAKS | BDRV_FIX_ERRORS; } else { - help(); + error_exit("Unknown option value for -r " + "(expecting 'leaks' or 'all'): %s", optarg); } break; case OPTION_OUTPUT: @@ -603,7 +618,7 @@ static int img_check(int argc, char **argv) } } if (optind != argc - 1) { - help(); + error_exit("Expecting one image file name"); } filename = argv[optind++]; @@ -714,7 +729,7 @@ static int img_commit(int argc, char **argv) } } if (optind != argc - 1) { - help(); + error_exit("Expecting one image file name"); } filename = argv[optind++]; @@ -960,7 +975,7 @@ static int img_compare(int argc, char **argv) if (optind != argc - 2) { - help(); + error_exit("Expecting two image file names"); } filename1 = argv[optind++]; filename2 = argv[optind++]; @@ -1276,7 +1291,7 @@ static int img_convert(int argc, char **argv) } if (bs_n < 1) { - help(); + error_exit("Must specify image file name"); } @@ -1886,7 +1901,7 @@ static int img_info(int argc, char **argv) } } if (optind != argc - 1) { - help(); + error_exit("Expecting one image file name"); } filename = argv[optind++]; @@ -2050,10 +2065,10 @@ static int img_map(int argc, char **argv) break; } } - if (optind >= argc) { - help(); + if (optind != argc - 1) { + error_exit("Expecting one image file name"); } - filename = argv[optind++]; + filename = argv[optind]; if (output && !strcmp(output, "json")) { output_format = OFORMAT_JSON; @@ -2142,7 +2157,7 @@ static int img_snapshot(int argc, char **argv) return 0; case 'l': if (action) { - help(); + error_exit("Cannot mix '-l', '-a', '-c', '-d'"); return 0; } action = SNAPSHOT_LIST; @@ -2150,7 +2165,7 @@ static int img_snapshot(int argc, char **argv) break; case 'a': if (action) { - help(); + error_exit("Cannot mix '-l', '-a', '-c', '-d'"); return 0; } action = SNAPSHOT_APPLY; @@ -2158,7 +2173,7 @@ static int img_snapshot(int argc, char **argv) break; case 'c': if (action) { - help(); + error_exit("Cannot mix '-l', '-a', '-c', '-d'"); return 0; } action = SNAPSHOT_CREATE; @@ -2166,7 +2181,7 @@ static int img_snapshot(int argc, char **argv) break; case 'd': if (action) { - help(); + error_exit("Cannot mix '-l', '-a', '-c', '-d'"); return 0; } action = SNAPSHOT_DELETE; @@ -2179,7 +2194,7 @@ static int img_snapshot(int argc, char **argv) } if (optind != argc - 1) { - help(); + error_exit("Expecting one image file name"); } filename = argv[optind++]; @@ -2292,8 +2307,11 @@ static int img_rebase(int argc, char **argv) progress = 0; } - if ((optind != argc - 1) || (!unsafe && !out_baseimg)) { - help(); + if (optind != argc - 1) { + error_exit("Expecting one image file name"); + } + if (!unsafe && !out_baseimg) { + error_exit("Must specify backing file (-b) or use unsafe mode (-u)"); } filename = argv[optind++]; @@ -2553,7 +2571,7 @@ static int img_resize(int argc, char **argv) /* Remove size from argv manually so that negative numbers are not treated * as options by getopt. */ if (argc < 3) { - help(); + error_exit("Not enough arguments"); return 1; } @@ -2580,7 +2598,7 @@ static int img_resize(int argc, char **argv) } } if (optind != argc - 1) { - help(); + error_exit("Expecting one image file name"); } filename = argv[optind++]; @@ -2697,7 +2715,7 @@ static int img_amend(int argc, char **argv) } if (!options) { - help(); + error_exit("Must specify options (-o)"); } filename = (optind == argc - 1) ? argv[argc - 1] : NULL; @@ -2709,7 +2727,7 @@ static int img_amend(int argc, char **argv) } if (optind != argc - 1) { - help(); + error_exit("Expecting one image file name"); } bs = bdrv_new_open("image", filename, fmt, @@ -2781,8 +2799,9 @@ int main(int argc, char **argv) qemu_init_main_loop(); bdrv_init(); - if (argc < 2) - help(); + if (argc < 2) { + error_exit("Not enough arguments"); + } cmdname = argv[1]; argc--; argv++; @@ -2794,6 +2813,5 @@ int main(int argc, char **argv) } /* not found */ - help(); - return 0; + error_exit("Command not found: %s", cmdname); } -- cgit v1.2.1 From 9b17031ac4efcf7a587f0e4eea82287a5329b6e7 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Thu, 17 Apr 2014 18:43:53 +0800 Subject: vmdk: Fix "%x" to PRIx32 in format strings for cid Signed-off-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/vmdk.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c index 938a183e66..06a1f9f93b 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -262,7 +262,7 @@ static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent) p_name = strstr(desc, cid_str); if (p_name != NULL) { p_name += cid_str_size; - sscanf(p_name, "%x", &cid); + sscanf(p_name, "%" SCNx32, &cid); } return cid; @@ -290,7 +290,7 @@ static int vmdk_write_cid(BlockDriverState *bs, uint32_t cid) p_name = strstr(desc, "CID"); if (p_name != NULL) { p_name += sizeof("CID"); - snprintf(p_name, sizeof(desc) - (p_name - desc), "%x\n", cid); + snprintf(p_name, sizeof(desc) - (p_name - desc), "%" PRIx32 "\n", cid); pstrcat(desc, sizeof(desc), tmp_desc); } @@ -1708,8 +1708,8 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options, const char desc_template[] = "# Disk DescriptorFile\n" "version=1\n" - "CID=%x\n" - "parentCID=%x\n" + "CID=%" PRIx32 "\n" + "parentCID=%" PRIx32 "\n" "createType=\"%s\"\n" "%s" "\n" @@ -1851,7 +1851,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options, } /* generate descriptor file */ desc = g_strdup_printf(desc_template, - (unsigned int)time(NULL), + (uint32_t)time(NULL), parent_cid, fmt, parent_desc_line, -- cgit v1.2.1 From 370e681629da587af7592a7b83ebc7ec4955461e Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 23 Apr 2014 10:05:20 +0200 Subject: block/cloop: use PRIu32 format specifier for uint32_t PRIu32 is the format string specifier for uint32_t, let's use it. Variables ->block_size, ->n_blocks, and i are all uint32_t. Suggested-by: Max Reitz Signed-off-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- block/cloop.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/block/cloop.c b/block/cloop.c index b6ad50fbb4..8457737922 100644 --- a/block/cloop.c +++ b/block/cloop.c @@ -72,7 +72,7 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags, } s->block_size = be32_to_cpu(s->block_size); if (s->block_size % 512) { - error_setg(errp, "block_size %u must be a multiple of 512", + error_setg(errp, "block_size %" PRIu32 " must be a multiple of 512", s->block_size); return -EINVAL; } @@ -86,7 +86,7 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags, * need a buffer this big. */ if (s->block_size > MAX_BLOCK_SIZE) { - error_setg(errp, "block_size %u must be %u MB or less", + error_setg(errp, "block_size %" PRIu32 " must be %u MB or less", s->block_size, MAX_BLOCK_SIZE / (1024 * 1024)); return -EINVAL; @@ -101,7 +101,7 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags, /* read offsets */ if (s->n_blocks > (UINT32_MAX - 1) / sizeof(uint64_t)) { /* Prevent integer overflow */ - error_setg(errp, "n_blocks %u must be %zu or less", + error_setg(errp, "n_blocks %" PRIu32 " must be %zu or less", s->n_blocks, (UINT32_MAX - 1) / sizeof(uint64_t)); return -EINVAL; @@ -133,7 +133,7 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags, if (s->offsets[i] < s->offsets[i - 1]) { error_setg(errp, "offsets not monotonically increasing at " - "index %u, image file is corrupt", i); + "index %" PRIu32 ", image file is corrupt", i); ret = -EINVAL; goto fail; } @@ -146,8 +146,8 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags, * ridiculous s->compressed_block allocation. */ if (size > 2 * MAX_BLOCK_SIZE) { - error_setg(errp, "invalid compressed block size at index %u, " - "image file is corrupt", i); + error_setg(errp, "invalid compressed block size at index %" PRIu32 + ", image file is corrupt", i); ret = -EINVAL; goto fail; } -- cgit v1.2.1