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

[Eric Blake]

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 <mreitz at redhat.com>
>> Signed-off-by: Eric Blake <eblake at 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?

> 
> But it does look like you got all cases covered this time.

Good to hear.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org