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 <mre...@redhat.com>
>>> Signed-off-by: Eric Blake <ebl...@redhat.com>
>>> ---
>
>>> +++ 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
-- sheepdog mailing list sheepdog@lists.wpkg.org https://lists.wpkg.org/mailman/listinfo/sheepdog
