There are several callers that need to create a new block backend from
an existing BDS; make the task slightly easier with a common helper
routine.
Suggested-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
include/sysemu/block-backend.h | 2 ++
block/block-backend.c | 23 +++++++++++++++++++++++
block/crypto.c | 8 +++-----
block/parallels.c | 7 +++----
block/qcow.c | 7 +++----
block/qcow2.c | 15 ++++++---------
block/qed.c | 7 +++----
block/sheepdog.c | 9 ++++-----
block/vdi.c | 7 +++----
block/vhdx.c | 7 +++----
block/vmdk.c | 9 ++++-----
block/vpc.c | 7 +++----
blockdev.c | 8 +++-----
blockjob.c | 7 ++-----
14 files changed, 65 insertions(+), 58 deletions(-)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 9bbdbd63d743..d37c1244dd9c 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -77,6 +77,8 @@ typedef struct BlockBackendPublic {
} BlockBackendPublic;
BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm);
+BlockBackend *blk_new_with_bs(BlockDriverState *bs, uint64_t perm,
+ uint64_t shared_perm, Error **errp);
BlockBackend *blk_new_open(const char *filename, const char *reference,
QDict *options, int flags, Error **errp);
int blk_get_refcnt(BlockBackend *blk);
diff --git a/block/block-backend.c b/block/block-backend.c
index 38ae41382652..1e082ce2a0f4 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -355,6 +355,29 @@ BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm)
return blk;
}
+/*
+ * Create a new BlockBackend connected to an existing BlockDriverState.
+ *
+ * @perm is a bitmasks of BLK_PERM_* constants which describes the permissions
+ * to request for a block driver node that is attached to this BlockBackend.
+ * @shared_perm is a bitmask which describes which permissions may be granted
+ * to other users of the attached node.
+ * Both sets of permissions can be changed later using blk_set_perm().
+ *
+ * Return the new BlockBackend on success, null on failure.
+ */
+BlockBackend *blk_new_with_bs(BlockDriverState *bs, uint64_t perm,
+ uint64_t shared_perm, Error **errp)
+{
+ BlockBackend *blk = blk_new(bdrv_get_aio_context(bs), perm, shared_perm);
+
+ if (blk_insert_bs(blk, bs, errp) < 0) {
+ blk_unref(blk);
+ return NULL;
+ }
+ return blk;
+}
+
/*
* Creates a new BlockBackend, opens a new BlockDriverState, and connects both.
* The new BlockBackend is in the main AioContext.
diff --git a/block/crypto.c b/block/crypto.c
index d577f89659fa..1cb8ae17ffde 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -261,11 +261,9 @@ static int block_crypto_co_create_generic(BlockDriverState *bs,
QCryptoBlock *crypto = NULL;
struct BlockCryptoCreateData data;
- blk = blk_new(bdrv_get_aio_context(bs),
- BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
-
- ret = blk_insert_bs(blk, bs, errp);
- if (ret < 0) {
+ blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+ errp);
+ if (!blk) {
goto cleanup;
}
diff --git a/block/parallels.c b/block/parallels.c
index 6d4ed77f165f..4019557ceee3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -559,10 +559,9 @@ static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
return -EIO;
}
- blk = blk_new(bdrv_get_aio_context(bs),
- BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
- ret = blk_insert_bs(blk, bs, errp);
- if (ret < 0) {
+ blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+ errp);
+ if (!blk) {
goto out;
}
blk_set_allow_write_beyond_eof(blk, true);
diff --git a/block/qcow.c b/block/qcow.c
index 8973e4e565a1..d9f26c515a49 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -849,10 +849,9 @@ static int coroutine_fn qcow_co_create(BlockdevCreateOptions *opts,
return -EIO;
}
- qcow_blk = blk_new(bdrv_get_aio_context(bs),
- BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
- ret = blk_insert_bs(qcow_blk, bs, errp);
- if (ret < 0) {
+ qcow_blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE,
+ BLK_PERM_ALL, errp);
+ if (!qcow_blk) {
goto exit;
}
blk_set_allow_write_beyond_eof(qcow_blk, true);
diff --git a/block/qcow2.c b/block/qcow2.c
index b524b0c53f84..1ce72041978b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3404,10 +3404,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
}
/* Create BlockBackend to write to the image */
- blk = blk_new(bdrv_get_aio_context(bs),
- BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
- ret = blk_insert_bs(blk, bs, errp);
- if (ret < 0) {
+ blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+ errp);
+ if (!blk) {
goto out;
}
blk_set_allow_write_beyond_eof(blk, true);
@@ -5359,11 +5358,9 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
}
if (new_size) {
- BlockBackend *blk = blk_new(bdrv_get_aio_context(bs),
- BLK_PERM_RESIZE, BLK_PERM_ALL);
- ret = blk_insert_bs(blk, bs, errp);
- if (ret < 0) {
- blk_unref(blk);
+ BlockBackend *blk = blk_new_with_bs(bs, BLK_PERM_RESIZE, BLK_PERM_ALL,
+ errp);
+ if (!blk) {
return ret;
}
diff --git a/block/qed.c b/block/qed.c
index 1af9b3cb1db1..dd767a5c75c9 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -651,10 +651,9 @@ static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts,
return -EIO;
}
- blk = blk_new(bdrv_get_aio_context(bs),
- BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
- ret = blk_insert_bs(blk, bs, errp);
- if (ret < 0) {
+ blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+ errp);
+ if (!blk) {
goto out;
}
blk_set_allow_write_beyond_eof(blk, true);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 59f7ebb1710f..09d75ff78dd8 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1803,12 +1803,11 @@ static int sd_prealloc(BlockDriverState *bs, int64_t old_size, int64_t new_size,
void *buf = NULL;
int ret;
- blk = blk_new(bdrv_get_aio_context(bs),
- BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_RESIZE,
- BLK_PERM_ALL);
+ blk = blk_new_with_bs(bs,
+ BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_RESIZE,
+ BLK_PERM_ALL, errp);
- ret = blk_insert_bs(blk, bs, errp);
- if (ret < 0) {
+ if (!blk) {
goto out_with_err_set;
}
diff --git a/block/vdi.c b/block/vdi.c
index e1a11f2aa097..d1b692741620 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -804,10 +804,9 @@ static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,
goto exit;
}
- blk = blk_new(bdrv_get_aio_context(bs_file),
- BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
- ret = blk_insert_bs(blk, bs_file, errp);
- if (ret < 0) {
+ blk = blk_new_with_bs(bs_file, BLK_PERM_WRITE | BLK_PERM_RESIZE,
+ BLK_PERM_ALL, errp);
+ if (!blk) {
goto exit;
}
diff --git a/block/vhdx.c b/block/vhdx.c
index 33e57cd6567a..4c908f226f48 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1984,10 +1984,9 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
return -EIO;
}
- blk = blk_new(bdrv_get_aio_context(bs),
- BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
- ret = blk_insert_bs(blk, bs, errp);
- if (ret < 0) {
+ blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+ errp);
+ if (!blk) {
goto delete_and_exit;
}
blk_set_allow_write_beyond_eof(blk, true);
diff --git a/block/vmdk.c b/block/vmdk.c
index 218d9c980059..dff9bd28cc3f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2717,11 +2717,10 @@ static BlockBackend *vmdk_co_create_cb(int64_t size, int idx,
if (!bs) {
return NULL;
}
- blk = blk_new(bdrv_get_aio_context(bs),
- BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_RESIZE,
- BLK_PERM_ALL);
- if (blk_insert_bs(blk, bs, errp)) {
- bdrv_unref(bs);
+ blk = blk_new_with_bs(bs,
+ BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_RESIZE,
+ BLK_PERM_ALL, errp);
+ if (!blk) {
return NULL;
}
blk_set_allow_write_beyond_eof(blk, true);
diff --git a/block/vpc.c b/block/vpc.c
index d8141b52da8b..250bb0bdad78 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1012,10 +1012,9 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
return -EIO;
}
- blk = blk_new(bdrv_get_aio_context(bs),
- BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
- ret = blk_insert_bs(blk, bs, errp);
- if (ret < 0) {
+ blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+ errp);
+ if (!blk) {
goto out;
}
blk_set_allow_write_beyond_eof(blk, true);
diff --git a/blockdev.c b/blockdev.c
index 5faddaa7052f..f43426ed5fbc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2711,7 +2711,6 @@ void qmp_block_resize(bool has_device, const char *device,
BlockBackend *blk = NULL;
BlockDriverState *bs;
AioContext *aio_context;
- int ret;
bs = bdrv_lookup_bs(has_device ? device : NULL,
has_node_name ? node_name : NULL,
@@ -2734,14 +2733,13 @@ void qmp_block_resize(bool has_device, const char *device,
goto out;
}
- blk = blk_new(bdrv_get_aio_context(bs), BLK_PERM_RESIZE, BLK_PERM_ALL);
- ret = blk_insert_bs(blk, bs, errp);
- if (ret < 0) {
+ blk = blk_new_with_bs(bs, BLK_PERM_RESIZE, BLK_PERM_ALL, errp);
+ if (!blk) {
goto out;
}
bdrv_drained_begin(bs);
- ret = blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp);
+ blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp);
bdrv_drained_end(bs);
out:
diff --git a/blockjob.c b/blockjob.c
index fc850312c124..2affa1844d4f 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -397,16 +397,13 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
{
BlockBackend *blk;
BlockJob *job;
- int ret;
if (job_id == NULL && !(flags & JOB_INTERNAL)) {
job_id = bdrv_get_device_name(bs);
}
- blk = blk_new(bdrv_get_aio_context(bs), perm, shared_perm);
- ret = blk_insert_bs(blk, bs, errp);
- if (ret < 0) {
- blk_unref(blk);
+ blk = blk_new_with_bs(bs, perm, shared_perm, errp);
+ if (!blk) {
return NULL;
}
--
2.26.2
On 24.04.20 00:17, Eric Blake wrote:
> There are several callers that need to create a new block backend from
> an existing BDS; make the task slightly easier with a common helper
> routine.
>
> Suggested-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> include/sysemu/block-backend.h | 2 ++
> block/block-backend.c | 23 +++++++++++++++++++++++
> block/crypto.c | 8 +++-----
> block/parallels.c | 7 +++----
> block/qcow.c | 7 +++----
> block/qcow2.c | 15 ++++++---------
> block/qed.c | 7 +++----
> block/sheepdog.c | 9 ++++-----
> block/vdi.c | 7 +++----
> block/vhdx.c | 7 +++----
> block/vmdk.c | 9 ++++-----
> block/vpc.c | 7 +++----
> blockdev.c | 8 +++-----
> blockjob.c | 7 ++-----
> 14 files changed, 65 insertions(+), 58 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
> diff --git a/blockdev.c b/blockdev.c
> index 5faddaa7052f..f43426ed5fbc 100644
> --- a/blockdev.c
> +++ b/blockdev.c
[...]
> @@ -2734,14 +2733,13 @@ void qmp_block_resize(bool has_device, const char *device,
> goto out;
> }
>
> - blk = blk_new(bdrv_get_aio_context(bs), BLK_PERM_RESIZE, BLK_PERM_ALL);
> - ret = blk_insert_bs(blk, bs, errp);
> - if (ret < 0) {
> + blk = blk_new_with_bs(bs, BLK_PERM_RESIZE, BLK_PERM_ALL, errp);
> + if (!blk) {
> goto out;
> }
>
> bdrv_drained_begin(bs);
> - ret = blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp);
> + blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp);
This is also addressed by
https://lists.nongnu.org/archive/html/qemu-devel/2020-04/msg03656.html,
but it does make sense to fix it here, too. Well, whichever lands first
lands first, I suppose (so @ret can be dropped).
Max
On 24.04.20 11:53, Max Reitz wrote:
> On 24.04.20 00:17, Eric Blake wrote:
>> There are several callers that need to create a new block backend from
>> an existing BDS; make the task slightly easier with a common helper
>> routine.
>>
>> Suggested-by: Max Reitz <mreitz@redhat.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> include/sysemu/block-backend.h | 2 ++
>> block/block-backend.c | 23 +++++++++++++++++++++++
>> block/crypto.c | 8 +++-----
>> block/parallels.c | 7 +++----
>> block/qcow.c | 7 +++----
>> block/qcow2.c | 15 ++++++---------
>> block/qed.c | 7 +++----
>> block/sheepdog.c | 9 ++++-----
>> block/vdi.c | 7 +++----
>> block/vhdx.c | 7 +++----
>> block/vmdk.c | 9 ++++-----
>> block/vpc.c | 7 +++----
>> blockdev.c | 8 +++-----
>> blockjob.c | 7 ++-----
>> 14 files changed, 65 insertions(+), 58 deletions(-)
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
(With the Patchew warning fixed, of course (i.e., we should set ret to
-EPERM or something in qcow.c))
>> diff --git a/blockdev.c b/blockdev.c
>> index 5faddaa7052f..f43426ed5fbc 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>
> [...]
>
>> @@ -2734,14 +2733,13 @@ void qmp_block_resize(bool has_device, const char *device,
>> goto out;
>> }
>>
>> - blk = blk_new(bdrv_get_aio_context(bs), BLK_PERM_RESIZE, BLK_PERM_ALL);
>> - ret = blk_insert_bs(blk, bs, errp);
>> - if (ret < 0) {
>> + blk = blk_new_with_bs(bs, BLK_PERM_RESIZE, BLK_PERM_ALL, errp);
>> + if (!blk) {
>> goto out;
>> }
>>
>> bdrv_drained_begin(bs);
>> - ret = blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp);
>> + blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp);
>
> This is also addressed by
> https://lists.nongnu.org/archive/html/qemu-devel/2020-04/msg03656.html,
> but it does make sense to fix it here, too. Well, whichever lands first
> lands first, I suppose (so @ret can be dropped).
I intended to affix the “(so @ret can be dropped)” to the first
sentence, not the second one...
Max
On 24.04.20 11:56, Max Reitz wrote: > On 24.04.20 11:53, Max Reitz wrote: >> On 24.04.20 00:17, Eric Blake wrote: >>> There are several callers that need to create a new block backend from >>> an existing BDS; make the task slightly easier with a common helper >>> routine. >>> >>> Suggested-by: Max Reitz <mreitz@redhat.com> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> --- >>> include/sysemu/block-backend.h | 2 ++ >>> block/block-backend.c | 23 +++++++++++++++++++++++ >>> block/crypto.c | 8 +++----- >>> block/parallels.c | 7 +++---- >>> block/qcow.c | 7 +++---- >>> block/qcow2.c | 15 ++++++--------- >>> block/qed.c | 7 +++---- >>> block/sheepdog.c | 9 ++++----- >>> block/vdi.c | 7 +++---- >>> block/vhdx.c | 7 +++---- >>> block/vmdk.c | 9 ++++----- >>> block/vpc.c | 7 +++---- >>> blockdev.c | 8 +++----- >>> blockjob.c | 7 ++----- >>> 14 files changed, 65 insertions(+), 58 deletions(-) >> >> Reviewed-by: Max Reitz <mreitz@redhat.com> > > (With the Patchew warning fixed, of course (i.e., we should set ret to > -EPERM or something in qcow.c)) Er, well, maybe I should have looked into more places. The compiler only warns about that single one because it’s the only place where @ret is really uninitialized, but there are many more where we need to set it: crypto.c, parallels.c, qcow.c, qcow2.c (both hunks), qed.c, sheepdog.c, vdi.c, vhdx.c, and vpc.c. (So basically everywhere but vmdk.c, blockdev.c, and blockjob.c.) And now I’m going to get another coffee... Max
On 4/24/20 5:02 AM, Max Reitz wrote: >> (With the Patchew warning fixed, of course (i.e., we should set ret to >> -EPERM or something in qcow.c)) > > Er, well, maybe I should have looked into more places. The compiler > only warns about that single one because it’s the only place where @ret > is really uninitialized, but there are many more where we need to set > it: crypto.c, parallels.c, qcow.c, qcow2.c (both hunks), qed.c, > sheepdog.c, vdi.c, vhdx.c, and vpc.c. > > (So basically everywhere but vmdk.c, blockdev.c, and blockjob.c.) Urgh - so it's even less of a win in terms of reduced lines of code. Still, I'll fix it and post v3. > > And now I’m going to get another coffee... > > Max > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 24.04.20 16:18, Eric Blake wrote: > On 4/24/20 5:02 AM, Max Reitz wrote: > >>> (With the Patchew warning fixed, of course (i.e., we should set ret to >>> -EPERM or something in qcow.c)) >> >> Er, well, maybe I should have looked into more places. The compiler >> only warns about that single one because it’s the only place where @ret >> is really uninitialized, but there are many more where we need to set >> it: crypto.c, parallels.c, qcow.c, qcow2.c (both hunks), qed.c, >> sheepdog.c, vdi.c, vhdx.c, and vpc.c. >> >> (So basically everywhere but vmdk.c, blockdev.c, and blockjob.c.) > > Urgh - so it's even less of a win in terms of reduced lines of code. True, but I still consider it a win in terms of reduced complexity. Before, we have two function calls with two variable assignments from their return values (@blk and @ret). Afterwards, we’ll have a single function all with one variable assignment from its return value (@blk), and one stand-alone variable assignment with a constant value (“ret = -EPERM” or similar). :) Max
On Thu, Apr 23, 2020 at 05:17:05PM -0500, Eric Blake wrote: > There are several callers that need to create a new block backend from > an existing BDS; make the task slightly easier with a common helper > routine. > > Suggested-by: Max Reitz <mreitz@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > include/sysemu/block-backend.h | 2 ++ > block/block-backend.c | 23 +++++++++++++++++++++++ > block/crypto.c | 8 +++----- > block/parallels.c | 7 +++---- > block/qcow.c | 7 +++---- > block/qcow2.c | 15 ++++++--------- > block/qed.c | 7 +++---- > block/sheepdog.c | 9 ++++----- > block/vdi.c | 7 +++---- > block/vhdx.c | 7 +++---- > block/vmdk.c | 9 ++++----- > block/vpc.c | 7 +++---- > blockdev.c | 8 +++----- > blockjob.c | 7 ++----- > 14 files changed, 65 insertions(+), 58 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
© 2016 - 2026 Red Hat, Inc.