Re: [PATCH v3 1/3] block: Add blk_new_with_bs() helper

[Max Reitz]

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 <address@hidden>
>>> Signed-off-by: Eric Blake <address@hidden>
>>> ---
> 
>>> +++ 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