On 27.04.20 16:03, Eric Blake wrote: > On 4/27/20 5:00 AM, Max Reitz wrote: >> On 24.04.20 21:09, 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 <[email protected]> >>> Signed-off-by: Eric Blake <[email protected]> >>> --- > >>> +++ b/block/crypto.c >>> @@ -256,16 +256,14 @@ static int >>> block_crypto_co_create_generic(BlockDriverState *bs, >>> PreallocMode prealloc, >>> Error **errp) >>> { >>> - int ret; >>> + int ret = -EPERM; >> >> I’m not sure I’m a fan of this, because I feel like it makes the code >> harder to read, due to having to look in three places (here, around the >> blk_new_with_bs() call, and under the cleanup label) instead of in two >> (not here) to verify that the error handling code is correct. >> >> There’s also the fact that this is not really a default return value, >> but one very specific error code for if one very specific function call >> fails. >> >> I suppose it comes down to whether one considers LoC a complexity >> problem. (I don’t, necessarily.) >> >> (Also I realize it seems rather common in the kernel to set error return >> variables before the function call, but I think the more common pattern >> in qemu is to set it in the error path.) > > I'm fine with either style. Setting it up front is handy if that > particular error makes a good default, but in many of the functions I > touched, we were returning a variety of errors (-EIO, -EINVAL, -EPERM, > etc) such that there was no good default, and thus no reason to set a > default up front. Is this something that would go through your tree, > and if so, are you okay making that tweak, or do I need to send v4?
I suppose I can do that, this is what I’d squash in, OK?
Max
diff --git a/block/crypto.c b/block/crypto.c
index a4d77f07fe..d09b364134 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -256,7 +256,7 @@ static int
block_crypto_co_create_generic(BlockDriverState *bs,
PreallocMode prealloc,
Error **errp)
{
- int ret = -EPERM;
+ int ret;
BlockBackend *blk;
QCryptoBlock *crypto = NULL;
struct BlockCryptoCreateData data;
@@ -264,6 +264,7 @@ static int
block_crypto_co_create_generic(BlockDriverState *bs,
blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE,
BLK_PERM_ALL,
errp);
if (!blk) {
+ ret = -EPERM;
goto cleanup;
}
diff --git a/block/qed.c b/block/qed.c
index 7a31495d29..671742511e 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -610,7 +610,7 @@ static int coroutine_fn
bdrv_qed_co_create(BlockdevCreateOptions *opts,
QEDHeader le_header;
uint8_t *l1_table = NULL;
size_t l1_size;
- int ret = -EPERM;
+ int ret = 0;
assert(opts->driver == BLOCKDEV_DRIVER_QED);
qed_opts = &opts->u.qed;
@@ -654,6 +654,7 @@ static int coroutine_fn
bdrv_qed_co_create(BlockdevCreateOptions *opts,
blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE,
BLK_PERM_ALL,
errp);
if (!blk) {
+ ret = -EPERM;
goto out;
}
blk_set_allow_write_beyond_eof(blk, true);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 2b53cd950d..8baf9e66bb 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1801,13 +1801,14 @@ static int sd_prealloc(BlockDriverState *bs,
int64_t old_size, int64_t new_size,
uint32_t idx, max_idx;
uint32_t object_size;
void *buf = NULL;
- int ret = -EPERM;
+ int ret;
blk = blk_new_with_bs(bs,
BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE |
BLK_PERM_RESIZE,
BLK_PERM_ALL, errp);
if (!blk) {
+ ret = -EPERM;
goto out_with_err_set;
}
diff --git a/block/vhdx.c b/block/vhdx.c
index bdf5d05cc0..8ca6976b59 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1891,7 +1891,7 @@ static int coroutine_fn
vhdx_co_create(BlockdevCreateOptions *opts,
BlockBackend *blk = NULL;
BlockDriverState *bs = NULL;
- int ret = -EPERM;
+ int ret = 0;
uint64_t image_size;
uint32_t log_size;
uint32_t block_size;
@@ -1987,6 +1987,7 @@ static int coroutine_fn
vhdx_co_create(BlockdevCreateOptions *opts,
blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE,
BLK_PERM_ALL,
errp);
if (!blk) {
+ ret = -EPERM;
goto delete_and_exit;
}
blk_set_allow_write_beyond_eof(blk, true);
signature.asc
Description: OpenPGP digital signature
